From 8b07b2b20ff2b3c5d9397fc0f894a961a1d7a46f Mon Sep 17 00:00:00 2001 From: Alex Reichert Date: Thu, 4 Feb 2021 14:32:21 -0500 Subject: [PATCH] Make it possible to sync an entire Slack thread (#561) * Improvements to how we handle syncing replies to bot messages (WIP) * Get everything working with syncing slack threads (WIP) * Play around with shuffling things around (WIP) --- .../conversations/ConversationsProvider.tsx | 11 +- .../conversations/SharedConversation.tsx | 6 +- assets/src/utils.ts | 20 + lib/chat_api/customers.ex | 72 ++- lib/chat_api/messages/notification.ex | 51 +- lib/chat_api/slack.ex | 49 ++ lib/chat_api/slack/client.ex | 16 + lib/chat_api/slack/event.ex | 138 ++--- lib/chat_api/slack/extractor.ex | 93 ++++ lib/chat_api/slack/helpers.ex | 489 ++++++++++++------ lib/chat_api/slack/notification.ex | 2 - lib/chat_api/slack/sync.ex | 187 +++++++ lib/chat_api/slack/validation.ex | 54 ++ lib/mix/tasks/set_missing_slack_user_ids.ex | 2 +- test/chat_api/customers_test.exs | 55 ++ test/chat_api/slack_test.exs | 123 ++++- .../controllers/slack_controller_test.exs | 14 +- 17 files changed, 1042 insertions(+), 340 deletions(-) create mode 100644 lib/chat_api/slack/extractor.ex create mode 100644 lib/chat_api/slack/sync.ex create mode 100644 lib/chat_api/slack/validation.ex diff --git a/assets/src/components/conversations/ConversationsProvider.tsx b/assets/src/components/conversations/ConversationsProvider.tsx index 6078a9192..aaf394ddd 100644 --- a/assets/src/components/conversations/ConversationsProvider.tsx +++ b/assets/src/components/conversations/ConversationsProvider.tsx @@ -4,7 +4,11 @@ import {throttle} from 'lodash'; import * as API from '../../api'; import {notification} from '../common'; import {Account, Conversation, Message, User} from '../../types'; -import {isWindowHidden, updateQueryParams} from '../../utils'; +import { + isWindowHidden, + sortConversationMessages, + updateQueryParams, +} from '../../utils'; import {SOCKET_URL} from '../../socket'; import logger from '../../logger'; @@ -523,10 +527,7 @@ export class ConversationsProvider extends React.Component { return { ...acc, - [conv.id]: messages.sort( - (a: Message, b: Message) => - +new Date(a.created_at) - +new Date(b.created_at) - ), + [conv.id]: sortConversationMessages(messages), }; }, {} diff --git a/assets/src/components/conversations/SharedConversation.tsx b/assets/src/components/conversations/SharedConversation.tsx index 091a90f48..f4bf1b949 100644 --- a/assets/src/components/conversations/SharedConversation.tsx +++ b/assets/src/components/conversations/SharedConversation.tsx @@ -4,6 +4,7 @@ import {Box, Flex} from 'theme-ui'; import qs from 'query-string'; import {colors} from '../common'; import ConversationMessages from './ConversationMessages'; +import {sortConversationMessages} from '../../utils'; import * as API from '../../api'; import {Message} from '../../types'; import logger from '../../logger'; @@ -35,10 +36,7 @@ class SharedConversationContainer extends React.Component { ); this.setState({ - messages: messages.sort( - (a: Message, b: Message) => - +new Date(a.created_at) - +new Date(b.created_at) - ), + messages: sortConversationMessages(messages), loading: false, }); } catch (err) { diff --git a/assets/src/utils.ts b/assets/src/utils.ts index 61e059f42..39670cebb 100644 --- a/assets/src/utils.ts +++ b/assets/src/utils.ts @@ -1,5 +1,9 @@ import dayjs from 'dayjs'; +import utc from 'dayjs/plugin/utc'; import qs from 'query-string'; +import {Message} from './types'; + +dayjs.extend(utc); const {REACT_APP_STRIPE_PUBLIC_KEY} = process.env; @@ -94,6 +98,22 @@ export const isValidUuid = (id: any) => { return regex.test(id); }; +export const sortConversationMessages = (messages: Array) => { + return messages.sort((a: Message, b: Message) => { + // NB: `created_at` is stored as UTC implicitly, whereas `sent_at` is stored + // as UTC explicitly. This means that we have to convert `created_at` to a + // UTC date on the frontend first in order to compare the two properly. + const dateA = a.sent_at + ? new Date(a.sent_at) + : dayjs.utc(a.created_at).toDate(); + const dateB = b.sent_at + ? new Date(b.sent_at) + : dayjs.utc(b.created_at).toDate(); + + return +dateA - +dateB; + }); +}; + export const updateQueryParams = (query: Record) => { if (window.history.pushState) { window.history.pushState( diff --git a/lib/chat_api/customers.ex b/lib/chat_api/customers.ex index 3e19aecbb..91e221556 100644 --- a/lib/chat_api/customers.ex +++ b/lib/chat_api/customers.ex @@ -58,6 +58,44 @@ defmodule ChatApi.Customers do external_id |> to_string() |> find_by_external_id(account_id, filters) end + @spec find_or_create_by_external_id(binary() | nil, binary(), map()) :: + {:ok, Customer.t()} | {:error, Ecto.Changeset.t()} | {:error, atom()} + def find_or_create_by_external_id(external_id, account_id, attrs \\ %{}) + def find_or_create_by_external_id(nil, _account_id, _attrs), do: {:error, :external_id_required} + + def find_or_create_by_external_id(external_id, account_id, attrs) do + case find_by_external_id(external_id, account_id) do + nil -> + get_default_params() + |> Map.merge(attrs) + |> Map.merge(%{external_id: external_id, account_id: account_id}) + |> create_customer() + + customer -> + {:ok, customer} + end + end + + @spec create_or_update_by_external_id(binary() | nil, binary(), map()) :: + {:ok, Customer.t()} | {:error, Ecto.Changeset.t()} | {:error, atom()} + def create_or_update_by_external_id(external_id, account_id, attrs \\ %{}) + + def create_or_update_by_external_id(nil, _account_id, _attrs), + do: {:error, :external_id_required} + + def create_or_update_by_external_id(external_id, account_id, attrs) do + case find_by_external_id(external_id, account_id) do + nil -> + get_default_params() + |> Map.merge(attrs) + |> Map.merge(%{external_id: external_id, account_id: account_id}) + |> create_customer() + + customer -> + update_customer(customer, attrs) + end + end + @spec find_by_email(binary() | nil, binary()) :: Customer.t() | nil def find_by_email(nil, _account_id), do: nil @@ -77,14 +115,7 @@ defmodule ChatApi.Customers do def find_or_create_by_email(email, account_id, attrs) do case find_by_email(email, account_id) do nil -> - %{ - # Defaults - first_seen: DateTime.utc_now(), - last_seen: DateTime.utc_now(), - # TODO: last_seen is stored as a date, while last_seen_at is stored as - # a datetime -- we should opt for datetime values whenever possible - last_seen_at: DateTime.utc_now() - } + get_default_params() |> Map.merge(attrs) |> Map.merge(%{email: email, account_id: account_id}) |> create_customer() @@ -102,14 +133,7 @@ defmodule ChatApi.Customers do def create_or_update_by_email(email, account_id, attrs) do case find_by_email(email, account_id) do nil -> - %{ - # Defaults - first_seen: DateTime.utc_now(), - last_seen: DateTime.utc_now(), - # TODO: last_seen is stored as a date, while last_seen_at is stored as - # a datetime -- we should opt for datetime values whenever possible - last_seen_at: DateTime.utc_now() - } + get_default_params() |> Map.merge(attrs) |> Map.merge(%{email: email, account_id: account_id}) |> create_customer() @@ -141,6 +165,22 @@ defmodule ChatApi.Customers do |> Repo.update() end + # Ideally these would be set at the DB level, but this should be fine for now + @spec get_default_params(map()) :: map() + def get_default_params(overrides \\ %{}) do + Map.merge( + %{ + # Defaults + first_seen: DateTime.utc_now(), + last_seen: DateTime.utc_now(), + # TODO: last_seen is stored as a date, while last_seen_at is stored as + # a datetime -- we should opt for datetime values whenever possible + last_seen_at: DateTime.utc_now() + }, + overrides + ) + end + # TODO: figure out if any of this can be done in the changeset, or if there's # a better way to handle this in general @spec sanitize_metadata(map()) :: map() diff --git a/lib/chat_api/messages/notification.ex b/lib/chat_api/messages/notification.ex index 5d79975db..27878f98f 100644 --- a/lib/chat_api/messages/notification.ex +++ b/lib/chat_api/messages/notification.ex @@ -34,21 +34,33 @@ defmodule ChatApi.Messages.Notification do message end - @spec notify(Message.t(), atom()) :: Message.t() + @spec notify(Message.t(), atom(), keyword()) :: Message.t() + def notify(message, type, opts \\ []) + def notify( %Message{body: _body, conversation_id: _conversation_id} = message, - :slack + :slack, + opts ) do Logger.info("Sending notification: :slack") - Task.start(fn -> - ChatApi.Slack.Notification.notify_primary_channel(message) - end) + case opts do + [metadata: %{"send_to_reply_channel" => false}] -> + nil + + [async: false] -> + ChatApi.Slack.Notification.notify_primary_channel(message) + + _ -> + Task.start(fn -> + ChatApi.Slack.Notification.notify_primary_channel(message) + end) + end message end - def notify(%Message{account_id: account_id} = message, :webhooks) do + def notify(%Message{account_id: account_id} = message, :webhooks, _opts) do Logger.info("Sending notification: :webhooks") # TODO: how should we handle errors/retry logic? Task.start(fn -> @@ -58,7 +70,7 @@ defmodule ChatApi.Messages.Notification do message end - def notify(%Message{} = message, :new_message_email) do + def notify(%Message{} = message, :new_message_email, _opts) do Logger.info("Sending notification: :new_message_email") # TODO: how should we handle errors/retry logic? Task.start(fn -> @@ -68,7 +80,7 @@ defmodule ChatApi.Messages.Notification do message end - def notify(%Message{} = message, :conversation_reply_email) do + def notify(%Message{} = message, :conversation_reply_email, _opts) do Logger.info("Sending notification: :conversation_reply_email") # 20 minutes (TODO: make this configurable?) schedule_in = 20 * 60 @@ -85,7 +97,7 @@ defmodule ChatApi.Messages.Notification do message end - def notify(%Message{} = message, :slack_company_channel) do + def notify(%Message{} = message, :slack_company_channel, _opts) do Logger.info("Sending notification: :slack_company_channel") Task.start(fn -> @@ -96,7 +108,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) do + def notify(%Message{} = message, :slack_support_channel, _opts) do Logger.info("Sending notification: :slack_support_channel") Task.start(fn -> @@ -106,28 +118,11 @@ defmodule ChatApi.Messages.Notification do message end - def notify(message, type) do + def notify(message, type, _opts) do Logger.error( "Unrecognized notification type #{inspect(type)} for message #{inspect(message)}" ) message end - - @spec notify(Message.t(), atom(), map()) :: Message.t() - def notify(%Message{} = message, :slack, metadata) do - # NB: we currently use the Slack authorization metadata to handle one-off cases - # where a user might not want to broadcast to the "reply"-type Slack integration - # under certain circumstances. - if send_to_slack_reply_channel?(metadata), - do: notify(message, :slack), - else: message - end - - def notify(%Message{} = message, type, _metadata) do - notify(message, type) - end - - defp send_to_slack_reply_channel?(%{"send_to_reply_channel" => false}), do: false - defp send_to_slack_reply_channel?(_), do: true end diff --git a/lib/chat_api/slack.ex b/lib/chat_api/slack.ex index f25a7e59b..ee74b96c8 100644 --- a/lib/chat_api/slack.ex +++ b/lib/chat_api/slack.ex @@ -2,4 +2,53 @@ defmodule ChatApi.Slack do @moduledoc """ A module to handle sending Slack notifications. """ + + # TODO: play around with using structs to format Slack API responses and webhook events + + defmodule Message do + @enforce_keys [:text] + + defstruct [ + :blocks, + :bot_id, + :client_msg_id, + :subtype, + :team, + :thread_ts, + :ts, + :user, + text: "", + type: "message" + ] + + def from_json(json) do + params = Map.new(json, fn {key, value} -> {String.to_atom(key), value} end) + + struct(Message, params) + end + end + + defmodule MessageEvent do + defstruct [ + :blocks, + :bot_id, + :client_msg_id, + :channel, + :channel_type, + :event_ts, + :subtype, + :team, + :thread_ts, + :ts, + :user, + text: "", + type: "message" + ] + + def from_json(json) do + params = Map.new(json, fn {key, value} -> {String.to_atom(key), value} end) + + struct(MessageEvent, params) + end + end end diff --git a/lib/chat_api/slack/client.ex b/lib/chat_api/slack/client.ex index b3f019645..b6377bd59 100644 --- a/lib/chat_api/slack/client.ex +++ b/lib/chat_api/slack/client.ex @@ -104,6 +104,22 @@ defmodule ChatApi.Slack.Client do end end + @spec retrieve_bot_info(binary(), binary()) :: {:ok, nil} | Tesla.Env.result() + def retrieve_bot_info(bot_id, access_token) do + if should_execute?(access_token) do + get("/bots.info", + query: [bot: bot_id], + headers: [ + {"Authorization", "Bearer " <> access_token} + ] + ) + else + Logger.info("Invalid access token") + + {:ok, nil} + end + end + @spec list_channels(binary()) :: {:ok, nil} | Tesla.Env.result() def list_channels(access_token) do # TODO: we need channels:read scope to access this diff --git a/lib/chat_api/slack/event.ex b/lib/chat_api/slack/event.ex index 90e26c06a..7eda697b9 100644 --- a/lib/chat_api/slack/event.ex +++ b/lib/chat_api/slack/event.ex @@ -53,7 +53,7 @@ defmodule ChatApi.Slack.Event do ) do Logger.debug("Handling Slack message reply event: #{inspect(event)}") - with {:ok, conversation} <- get_thread_conversation(thread_ts, slack_channel_id), + with {:ok, conversation} <- find_thread_conversation(thread_ts, slack_channel_id), %{account_id: account_id, id: conversation_id} <- conversation, primary_reply_authorization <- SlackAuthorizations.get_authorization_by_account(account_id, %{type: "reply"}) do @@ -103,10 +103,10 @@ defmodule ChatApi.Slack.Event do end end else - # If an existing conversation is not found, we check to see if this is a reply to a bot message. - # At the moment, we want to start a new thread for replies to bot messages. + # If an existing conversation is not found, we check to see if this is a reply to a bot + # or agent message. At the moment, we want to start a new thread for replies to these messages. {:error, :not_found} -> - handle_reply_to_bot_event(event) + handle_reply_to_unknown_thread(event) error -> error @@ -138,9 +138,9 @@ defmodule ChatApi.Slack.Event do # This validates that the channel doesn't match the initially connected channel on # the `slack_authorization` record, since we currently treat that channel slightly differently true <- channel_id != slack_channel_id, - :ok <- validate_no_existing_company(account_id, slack_channel_id), + :ok <- Slack.Validation.validate_no_existing_company(account_id, slack_channel_id), {:ok, response} <- Slack.Client.retrieve_channel_info(slack_channel_id, access_token), - {:ok, channel} <- Slack.Helpers.extract_slack_channel(response), + {:ok, channel} <- Slack.Extractor.extract_slack_channel(response), %{"name" => name, "purpose" => purpose, "topic" => topic} <- channel do company = %{ # Set default company name to Slack channel name @@ -183,8 +183,8 @@ defmodule ChatApi.Slack.Event do }), # TODO: remove after debugging! :ok <- Logger.info("Handling Slack new message event: #{inspect(event)}"), - :ok <- validate_channel_supported(authorization, slack_channel_id), - :ok <- validate_non_admin_user(authorization, slack_user_id) do + :ok <- Slack.Validation.validate_channel_supported(authorization, slack_channel_id), + :ok <- Slack.Validation.validate_non_admin_user(authorization, slack_user_id) do create_new_conversation_from_slack_message(event, authorization) end end @@ -203,12 +203,12 @@ defmodule ChatApi.Slack.Event do ) do Logger.info("Handling Slack reaction event: #{inspect(event)}") - with :ok <- validate_no_existing_thread(channel, ts), + with :ok <- Slack.Validation.validate_no_existing_thread(channel, ts), {:ok, account_id} <- find_account_id_by_support_channel(channel), %{access_token: access_token} <- SlackAuthorizations.get_authorization_by_account(account_id, %{type: "support"}), {:ok, response} <- Slack.Client.retrieve_message(channel, ts, access_token), - {:ok, message} <- Slack.Helpers.extract_slack_message(response) do + {:ok, message} <- Slack.Extractor.extract_slack_message(response) do Logger.info("Slack emoji reaction detected:") Logger.info(inspect(event)) @@ -239,68 +239,38 @@ defmodule ChatApi.Slack.Event do team_id: team, type: "support" }), - :ok <- validate_channel_supported(authorization, slack_channel_id) do + :ok <- Slack.Validation.validate_channel_supported(authorization, slack_channel_id) do + # TODO: sync whole message thread if there are multiple messages already + # (See `Slack.Sync.sync_slack_message_thread(messages, authorization, event)`) create_new_conversation_from_slack_message(event, authorization) end end - @spec handle_reply_to_bot_event(map()) :: any() - def handle_reply_to_bot_event( + def handle_emoji_reaction_event(_), do: nil + + @spec handle_reply_to_unknown_thread(map()) :: any() + def handle_reply_to_unknown_thread( %{ "type" => "message", "text" => _text, "team" => team, - "thread_ts" => thread_ts, - "channel" => slack_channel_id, - "user" => slack_user_id + "thread_ts" => _thread_ts, + "channel" => _slack_channel_id, + "user" => _slack_user_id } = event ) do - with %{access_token: access_token} = authorization <- + with %SlackAuthorization{} = authorization <- SlackAuthorizations.find_slack_authorization(%{ team_id: team, type: "support" }), - # TODO: remove after debugging! - :ok <- Logger.info("Checking if message event is reply to bot: #{inspect(event)}"), - :ok <- validate_channel_supported(authorization, slack_channel_id), - {:ok, response} <- - Slack.Client.retrieve_conversation_replies(slack_channel_id, thread_ts, access_token), - {:ok, [initial_message | replies]} <- Slack.Helpers.extract_slack_messages(response), - # TODO: support both bot messages AND messages from admin/internal users - true <- Slack.Helpers.is_bot_message?(initial_message) do - # Handle initial message first - create_new_conversation_from_slack_message( - %{ - "type" => "message", - "text" => Map.get(initial_message, "text"), - "channel" => slack_channel_id, - # TODO: this will currently treat the bot as if it were the user... - # We still need to add better support for bot messages - "user" => Map.get(initial_message, "user", slack_user_id), - "ts" => thread_ts - }, - authorization - ) - - # Then, handle replies - Enum.each(replies, fn msg -> - # Wait 1s between each message so they don't have the same `inserted_at` - # timestamp... in the future, we should start sorting by `sent_at` instead! - Process.sleep(1000) - - handle_event(%{ - "type" => "message", - "text" => Map.get(msg, "text"), - "ts" => Map.get(msg, "ts"), - "thread_ts" => thread_ts, - "channel" => slack_channel_id, - "user" => Map.get(msg, "user", slack_user_id) - }) - end) + [_ | _] = messages <- Slack.Sync.get_syncable_slack_messages(authorization, event), + true <- Slack.Sync.should_sync_slack_messages?(messages) do + Slack.Sync.sync_slack_message_thread(messages, authorization, event) end end - def handle_reply_to_bot_event(_event), do: nil + def handle_reply_to_unknown_thread(_event), do: nil # TODO: move to Slack.Helpers? @spec create_new_conversation_from_slack_message(map(), SlackAuthorization.t()) :: @@ -310,19 +280,14 @@ defmodule ChatApi.Slack.Event do "type" => "message", "text" => text, "channel" => slack_channel_id, - "user" => slack_user_id, "ts" => ts - } = _event, + } = event, %SlackAuthorization{account_id: account_id} = authorization ) do # NB: not ideal, but this may treat an internal/admin user as a "customer", # because at the moment all conversations must have a customer associated with them with {:ok, customer} <- - Slack.Helpers.create_or_update_customer_from_slack_user_id( - authorization, - slack_user_id, - slack_channel_id - ), + Slack.Helpers.create_or_update_customer_from_slack_event(authorization, event), # TODO: should the conversation + thread + message all be handled in a transaction? # Probably yes at some point, but for now... not too big a deal ¯\_(ツ)_/¯ # TODO: should we handle default assignment here as well? @@ -359,11 +324,13 @@ defmodule ChatApi.Slack.Event do # TODO: should we make this configurable? Or only do it from private channels? # (Leaving this enabled for the emoji reaction use case, since it's an explicit action # as opposed to the auto-syncing that occurs above for all new messages) - |> Messages.Notification.notify(:slack, authorization.metadata) + |> Messages.Notification.notify(:slack, metadata: authorization.metadata) end end - defp get_thread_conversation(thread_ts, channel) do + @spec find_thread_conversation(binary(), binary()) :: + {:ok, Conversation.t()} | {:error, :not_found} + defp find_thread_conversation(thread_ts, channel) do case SlackConversationThreads.get_by_slack_thread_ts(thread_ts, channel) do %{conversation: conversation} -> {:ok, conversation} _ -> {:error, :not_found} @@ -386,47 +353,4 @@ defmodule ChatApi.Slack.Event do end end end - - @spec validate_non_admin_user(any(), binary()) :: :ok | :error - defp validate_non_admin_user(authorization, slack_user_id) do - case Slack.Helpers.find_matching_user(authorization, slack_user_id) do - nil -> :ok - _match -> :error - end - end - - @spec validate_channel_supported(any(), binary()) :: :ok | :error - defp validate_channel_supported( - %SlackAuthorization{channel_id: slack_channel_id}, - slack_channel_id - ), - do: :ok - - defp validate_channel_supported( - %SlackAuthorization{account_id: account_id}, - slack_channel_id - ) do - case ChatApi.Companies.find_by_slack_channel(account_id, slack_channel_id) do - nil -> :error - _company -> :ok - end - end - - defp validate_channel_supported(_authorization, _slack_channel_id), do: :error - - @spec validate_no_existing_company(binary(), binary()) :: :ok | :error - def validate_no_existing_company(account_id, slack_channel_id) do - case ChatApi.Companies.find_by_slack_channel(account_id, slack_channel_id) do - nil -> :ok - _company -> :error - end - end - - @spec validate_no_existing_thread(binary(), binary()) :: :ok | :error - def validate_no_existing_thread(channel, ts) do - case SlackConversationThreads.exists?(%{"slack_thread_ts" => ts, "slack_channel" => channel}) do - false -> :ok - true -> :error - end - end end diff --git a/lib/chat_api/slack/extractor.ex b/lib/chat_api/slack/extractor.ex new file mode 100644 index 000000000..8350c7667 --- /dev/null +++ b/lib/chat_api/slack/extractor.ex @@ -0,0 +1,93 @@ +defmodule ChatApi.Slack.Extractor do + require Logger + + @spec extract_slack_message(map()) :: {:ok, map()} | {:error, String.t()} + def extract_slack_message(%{body: %{"ok" => true, "messages" => [message | _]}}), + do: {:ok, message} + + def extract_slack_message(%{body: %{"ok" => true, "messages" => []}}), + do: {:error, "No messages were found"} + + def extract_slack_message(%{body: %{"ok" => false} = body}) do + Logger.error("conversations.history returned ok=false: #{inspect(body)}") + + {:error, "conversations.history returned ok=false: #{inspect(body)}"} + end + + def extract_slack_message(response), + do: {:error, "Invalid response: #{inspect(response)}"} + + @spec extract_slack_messages(map()) :: {:ok, [map()]} | {:error, String.t()} + def extract_slack_messages(%{body: %{"ok" => true, "messages" => messages}}) + when is_list(messages), + do: {:ok, messages} + + def extract_slack_messages(%{body: %{"ok" => false} = body}) do + Logger.error("conversations.replies returned ok=false: #{inspect(body)}") + + {:error, "conversations.replies returned ok=false: #{inspect(body)}"} + end + + def extract_slack_messages(response), + do: {:error, "Invalid response: #{inspect(response)}"} + + @spec extract_slack_channel(map()) :: {:ok, map()} | {:error, String.t()} + def extract_slack_channel(%{body: %{"ok" => true, "channel" => channel}}) when is_map(channel), + do: {:ok, channel} + + def extract_slack_channel(%{body: %{"ok" => false} = body}) do + Logger.error("conversations.info returned ok=false: #{inspect(body)}") + + {:error, "conversations.info returned ok=false: #{inspect(body)}"} + end + + def extract_slack_channel(response), + do: {:error, "Invalid response: #{inspect(response)}"} + + @slackbot_user_id "USLACKBOT" + + @spec extract_valid_slack_users(map()) :: {:ok, [map()]} | {:error, String.t()} + def extract_valid_slack_users(%{body: %{"ok" => true, "members" => members}}) do + users = + Enum.reject(members, fn member -> + Map.get(member, "is_bot") || + Map.get(member, "deleted") || + member["id"] == @slackbot_user_id + end) + + {:ok, users} + end + + def extract_valid_slack_users(%{body: %{"ok" => true, "members" => []}}), + do: {:error, "No users were found"} + + def extract_valid_slack_users(response), + do: {:error, "Invalid response: #{inspect(response)}"} + + # TODO: refactor extractors below to return :ok/:error tuples rather than raising? + + @spec extract_slack_conversation_thread_info!(map()) :: map() + def extract_slack_conversation_thread_info!(%{body: body}) do + if Map.get(body, "ok") do + %{ + slack_channel: Map.get(body, "channel"), + slack_thread_ts: Map.get(body, "ts") + } + else + Logger.error("Error sending Slack message: #{inspect(body)}") + + raise "chat.postMessage returned ok=false" + end + end + + @spec extract_slack_user_email!(map()) :: binary() + def extract_slack_user_email!(%{body: body}) do + if Map.get(body, "ok") do + get_in(body, ["user", "profile", "email"]) + else + Logger.error("Error retrieving user info: #{inspect(body)}") + + raise "users.info returned ok=false" + end + end +end diff --git a/lib/chat_api/slack/helpers.ex b/lib/chat_api/slack/helpers.ex index 841ba8a50..27ca7905d 100644 --- a/lib/chat_api/slack/helpers.ex +++ b/lib/chat_api/slack/helpers.ex @@ -32,7 +32,7 @@ defmodule ChatApi.Slack.Helpers do {:ok, response} -> try do - extract_slack_user_email(response) + Slack.Extractor.extract_slack_user_email!(response) rescue error -> Logger.error("Unable to retrieve Slack user email: #{inspect(error)}") @@ -73,27 +73,58 @@ defmodule ChatApi.Slack.Helpers do end end + @spec find_or_create_customer_from_slack_event(SlackAuthorization.t(), map()) :: + {:ok, Customer.t()} | {:error, any()} + def find_or_create_customer_from_slack_event(authorization, %{ + "channel" => slack_channel_id, + "user" => slack_user_id + }) + when not is_nil(slack_user_id) and not is_nil(slack_channel_id) do + find_or_create_customer_from_slack_user_id(authorization, slack_user_id, slack_channel_id) + end + + def find_or_create_customer_from_slack_event(authorization, %{"bot" => slack_bot_id}) + when not is_nil(slack_bot_id) do + find_or_create_customer_from_slack_bot_id(authorization, slack_bot_id) + end + + @spec find_or_create_customer_from_slack_bot_id(any(), binary()) :: + {:ok, Customer.t()} | {:error, any()} + def find_or_create_customer_from_slack_bot_id(authorization, slack_bot_id) do + with %{access_token: access_token, account_id: account_id} <- authorization, + {:ok, %{body: %{"ok" => true, "bot" => bot}}} <- + Slack.Client.retrieve_bot_info(slack_bot_id, access_token) do + attrs = customer_params_for_slack_bot(bot) + + Customers.find_or_create_by_external_id(slack_bot_id, account_id, attrs) + else + # NB: This may occur in test mode, or when the Slack.Client is disabled + {:ok, error} -> + Logger.error("Error creating customer from Slack bot user: #{inspect(error)}") + + error + + error -> + Logger.error("Error creating customer from Slack bot user: #{inspect(error)}") + + error + end + end + @spec find_or_create_customer_from_slack_user_id(any(), binary(), binary()) :: {:ok, Customer.t()} | {:error, any()} def find_or_create_customer_from_slack_user_id(authorization, slack_user_id, slack_channel_id) do with %{access_token: access_token, account_id: account_id} <- authorization, {:ok, %{body: %{"ok" => true, "user" => user}}} <- Slack.Client.retrieve_user_info(slack_user_id, access_token), - %{"profile" => %{"email" => email} = profile} <- user do + %{"profile" => %{"email" => email}} <- user do company_attrs = case Companies.find_by_slack_channel(account_id, slack_channel_id) do %{id: company_id} -> %{company_id: company_id} _ -> %{} end - attrs = - %{ - name: Map.get(profile, "real_name"), - time_zone: Map.get(user, "tz") - } - |> Enum.reject(fn {_k, v} -> is_nil(v) end) - |> Map.new() - |> Map.merge(company_attrs) + attrs = customer_params_for_slack_user(user, company_attrs) Customers.find_or_create_by_email(email, account_id, attrs) else @@ -110,6 +141,42 @@ defmodule ChatApi.Slack.Helpers do end end + @spec create_or_update_customer_from_slack_event(SlackAuthorization.t(), map()) :: + {:ok, Customer.t()} | {:error, any()} + def create_or_update_customer_from_slack_event(authorization, %{ + "channel" => slack_channel_id, + "user" => slack_user_id + }) + when not is_nil(slack_user_id) and not is_nil(slack_channel_id) do + create_or_update_customer_from_slack_user_id(authorization, slack_user_id, slack_channel_id) + end + + def create_or_update_customer_from_slack_event(authorization, %{"bot" => slack_bot_id}) + when not is_nil(slack_bot_id) do + create_or_update_customer_from_slack_bot_id(authorization, slack_bot_id) + end + + @spec create_or_update_customer_from_slack_bot_id(any(), binary()) :: + {:ok, Customer.t()} | {:error, any()} + def create_or_update_customer_from_slack_bot_id(authorization, slack_bot_id) do + with %{access_token: access_token, account_id: account_id} <- authorization, + {:ok, %{body: %{"ok" => true, "bot" => bot}}} <- + Slack.Client.retrieve_bot_info(slack_bot_id, access_token) do + create_or_update_customer_from_slack_bot(bot, account_id) + else + # NB: This may occur in test mode, or when the Slack.Client is disabled + {:ok, error} -> + Logger.error("Error creating customer from Slack bot user: #{inspect(error)}") + + error + + error -> + Logger.error("Error creating customer from Slack bot user: #{inspect(error)}") + + error + end + end + # NB: this is basically the same as `find_or_create_customer_from_slack_user_id` above, # but keeping both with duplicate code for now since we may get rid of one in the near future @spec create_or_update_customer_from_slack_user_id(any(), binary(), binary()) :: @@ -117,25 +184,14 @@ defmodule ChatApi.Slack.Helpers do def create_or_update_customer_from_slack_user_id(authorization, slack_user_id, slack_channel_id) do with %{access_token: access_token, account_id: account_id} <- authorization, {:ok, %{body: %{"ok" => true, "user" => user}}} <- - Slack.Client.retrieve_user_info(slack_user_id, access_token), - %{"profile" => %{"email" => email} = profile} <- user do - company_attrs = - case Companies.find_by_slack_channel(account_id, slack_channel_id) do - %{id: company_id} -> %{company_id: company_id} - _ -> %{} - end - - attrs = - %{ - name: Map.get(profile, "real_name"), - time_zone: Map.get(user, "tz"), - profile_photo_url: Map.get(profile, "image_original") - } - |> Enum.reject(fn {_k, v} -> is_nil(v) end) - |> Map.new() - |> Map.merge(company_attrs) + Slack.Client.retrieve_user_info(slack_user_id, access_token) do + case Companies.find_by_slack_channel(account_id, slack_channel_id) do + %{id: company_id} -> + create_or_update_customer_from_slack_user(user, account_id, %{company_id: company_id}) - Customers.create_or_update_by_email(email, account_id, attrs) + _ -> + create_or_update_customer_from_slack_user(user, account_id) + end else # NB: This may occur in test mode, or when the Slack.Client is disabled {:ok, error} -> @@ -150,32 +206,123 @@ defmodule ChatApi.Slack.Helpers do end end - @spec find_matching_customer(any(), binary()) :: Customer.t() | nil - def find_matching_customer(authorization, slack_user_id) do - case authorization do - %{access_token: access_token, account_id: account_id} -> - slack_user_id - |> get_user_email(access_token) - |> Customers.find_by_email(account_id) + @spec create_or_update_customer_from_slack_user_id(any(), binary()) :: + {:ok, Customer.t()} | {:error, any()} + def create_or_update_customer_from_slack_user_id(authorization, slack_user_id) do + with %{access_token: access_token, account_id: account_id} <- authorization, + {:ok, %{body: %{"ok" => true, "user" => user}}} <- + Slack.Client.retrieve_user_info(slack_user_id, access_token) do + create_or_update_customer_from_slack_user(user, account_id) + else + # NB: This may occur in test mode, or when the Slack.Client is disabled + {:ok, error} -> + Logger.error("Error creating customer from Slack user: #{inspect(error)}") - _ -> - nil + error + + error -> + Logger.error("Error creating customer from Slack user: #{inspect(error)}") + + error end end - @spec find_matching_user(any(), binary()) :: User.t() | nil - def find_matching_user(authorization, slack_user_id) do - case authorization do - %{access_token: access_token, account_id: account_id} -> + @spec customer_params_for_slack_user(map(), map()) :: map() + def customer_params_for_slack_user(slack_user, attrs \\ %{}) + + def customer_params_for_slack_user(%{"profile" => profile} = slack_user, attrs) do + %{ + name: Map.get(profile, "real_name"), + time_zone: Map.get(slack_user, "tz"), + profile_photo_url: Map.get(profile, "image_original") + } + |> Enum.reject(fn {_k, v} -> is_nil(v) end) + |> Map.new() + |> Map.merge(attrs) + end + + def customer_params_for_slack_user(slack_user, _attrs) do + Logger.error("Unexpected Slack user: #{inspect(slack_user)}") + + %{} + end + + @spec create_or_update_customer_from_slack_user(map(), binary(), map()) :: + {:ok, Customer.t()} | {:error, any()} + def create_or_update_customer_from_slack_user(slack_user, account_id, attrs \\ %{}) + + def create_or_update_customer_from_slack_user( + %{"profile" => %{"email" => email}} = slack_user, + account_id, + attrs + ) do + params = customer_params_for_slack_user(slack_user, attrs) + + Customers.create_or_update_by_email(email, account_id, params) + end + + def create_or_update_customer_from_slack_user(slack_user, _account_id, _attrs) do + {:error, "Invalid Slack user: #{inspect(slack_user)}"} + end + + @spec customer_params_for_slack_bot(map()) :: map() + def customer_params_for_slack_bot(slack_bot) do + %{ + name: Map.get(slack_bot, "name"), + profile_photo_url: get_in(slack_bot, ["icons", "image_72"]) + } + |> Enum.reject(fn {_k, v} -> is_nil(v) end) + |> Map.new() + end + + @spec create_or_update_customer_from_slack_bot(map(), binary()) :: + {:ok, Customer.t()} | {:error, any()} + def create_or_update_customer_from_slack_bot(slack_bot, account_id) + + def create_or_update_customer_from_slack_bot( + %{"id" => slack_bot_id} = slack_bot, + account_id + ) do + params = customer_params_for_slack_bot(slack_bot) + + Customers.create_or_update_by_external_id(slack_bot_id, account_id, params) + end + + def create_or_update_customer_from_slack_bot(slack_bot, _account_id) do + {:error, "Invalid Slack bot: #{inspect(slack_bot)}"} + end + + @spec find_matching_customer(SlackAuthorization.t() | nil, binary()) :: Customer.t() | nil + def find_matching_customer( + %SlackAuthorization{access_token: access_token, account_id: account_id}, slack_user_id - |> get_user_email(access_token) - |> Users.find_user_by_email(account_id) + ) do + slack_user_id + |> get_user_email(access_token) + |> Customers.find_by_email(account_id) + end - _ -> - nil - end + def find_matching_customer(_authorization, _slack_user_id), do: nil + + @spec find_matching_user(SlackAuthorization.t(), binary()) :: User.t() | nil + def find_matching_user( + %SlackAuthorization{access_token: access_token, account_id: account_id}, + slack_user_id + ) do + slack_user_id + |> get_user_email(access_token) + |> Users.find_user_by_email(account_id) + end + + def find_matching_user(_authorization, _slack_user_id), do: nil + + @spec find_matching_bot_customer(any(), binary()) :: Customer.t() | nil + def find_matching_bot_customer(%SlackAuthorization{account_id: account_id}, slack_bot_id) do + Customers.find_by_external_id(slack_bot_id, account_id) end + def find_matching_bot_customer(_authorization, _slack_bot_id), do: nil + @spec get_admin_sender_id(any(), binary(), binary()) :: binary() def get_admin_sender_id(authorization, slack_user_id, fallback) do case find_matching_user(authorization, slack_user_id) do @@ -184,6 +331,109 @@ defmodule ChatApi.Slack.Helpers do end end + @doc """ + Checks for a matching `User` for the Slack message event if the accumulator is `nil`. + + If a matching `User` or `Customer` has already been found, just return it. + """ + @spec maybe_find_user(User.t() | Customer.t() | nil, SlackAuthorization.t(), map()) :: + User.t() | Customer.t() | nil + def maybe_find_user(nil, authorization, %{"user" => slack_user_id}) do + find_matching_user(authorization, slack_user_id) + end + + def maybe_find_user(%User{} = user, _, _), do: user + def maybe_find_user(%Customer{} = customer, _, _), do: customer + def maybe_find_user(nil, _, _), do: nil + + @doc """ + Checks for a matching `Customer` for the Slack message event if the accumulator is `nil`. + + If a matching `User` or `Customer` has already been found, just return it. + """ + @spec maybe_find_user(User.t() | Customer.t() | nil, SlackAuthorization.t(), map()) :: + User.t() | Customer.t() | nil + def maybe_find_customer(nil, authorization, %{"bot_id" => slack_bot_id}) do + find_matching_bot_customer(authorization, slack_bot_id) + end + + def maybe_find_customer(nil, authorization, %{"user" => slack_user_id}) do + find_matching_customer(authorization, slack_user_id) + end + + def maybe_find_customer(%Customer{} = customer, _, _), do: customer + def maybe_find_customer(%User{} = user, _, _), do: user + def maybe_find_customer(nil, _, _), do: nil + + @doc """ + Fetches the matching `User` or `Customer` for the Slack message event. + """ + @spec get_sender_info(SlackAuthorization.t(), map()) :: User.t() | Customer.t() | nil + def get_sender_info(authorization, slack_message) do + nil + |> maybe_find_user(authorization, slack_message) + |> maybe_find_customer(authorization, slack_message) + |> case do + %User{} = user -> user + %Customer{} = customer -> customer + _ -> nil + end + end + + @doc """ + Updates the params with a "user_id" field if a "customer_id" has not already been set. + """ + @spec maybe_set_user_id(map(), SlackAuthorization.t(), map()) :: map() + def maybe_set_user_id(%{"customer_id" => customer_id} = params, _authorization, _event) + when not is_nil(customer_id), + do: params + + def maybe_set_user_id(params, authorization, %{"user" => slack_user_id}) do + case find_matching_user(authorization, slack_user_id) do + %User{id: user_id} -> + Map.merge(params, %{"user_id" => user_id}) + + _ -> + params + end + end + + def maybe_set_user_id(params, _authorization, _event), do: params + + @doc """ + Updates the params with a "customer_id" field if a "user_id" has not already been set. + """ + @spec maybe_set_customer_id(map(), SlackAuthorization.t(), map()) :: map() + def maybe_set_customer_id(%{"user_id" => user_id} = params, _authorization, _event) + when not is_nil(user_id), + do: params + + def maybe_set_customer_id(params, authorization, event) do + case create_or_update_customer_from_slack_event(authorization, event) do + {:ok, %Customer{id: customer_id}} -> + Map.merge(params, %{"customer_id" => customer_id}) + + _ -> + params + end + end + + @spec format_sender_id_v2!(SlackAuthorization.t(), map()) :: map() + def format_sender_id_v2!(authorization, event) do + %{} + |> maybe_set_user_id(authorization, event) + |> maybe_set_customer_id(authorization, event) + |> case do + params when map_size(params) == 1 -> + params + + _invalid -> + raise "Unable to find matching user or customer ID for Slack event #{inspect(event)} on account authorization #{ + inspect(authorization) + }" + end + end + @spec format_sender_id!(any(), binary(), binary()) :: map() def format_sender_id!(authorization, slack_user_id, slack_channel_id) do # TODO: what's the best way to handle these nested `case` statements? @@ -199,7 +449,7 @@ defmodule ChatApi.Slack.Helpers do %{"customer_id" => customer_id} _ -> - case find_or_create_customer_from_slack_user_id( + case create_or_update_customer_from_slack_user_id( authorization, slack_user_id, slack_channel_id @@ -232,26 +482,6 @@ defmodule ChatApi.Slack.Helpers do def is_private_slack_channel?("C" <> _rest), do: false def is_private_slack_channel?(_), do: false - @spec get_slack_authorization(binary()) :: - %{access_token: binary(), channel: binary(), channel_id: binary()} - | SlackAuthorization.t() - def get_slack_authorization(account_id) do - case SlackAuthorizations.get_authorization_by_account(account_id) do - # Supports a fallback access token as an env variable to make it easier to - # test locally (assumes the existence of a "bots" channel in your workspace) - # TODO: deprecate - nil -> - %{ - access_token: Slack.Token.get_default_access_token(), - channel: "#bots", - channel_id: "1" - } - - auth -> - auth - end - end - # TODO: not sure the most idiomatic way to handle this, but basically this # just formats how we show the name/email of the customer if they exist @spec identify_customer(Customer.t()) :: binary() @@ -274,11 +504,12 @@ defmodule ChatApi.Slack.Helpers do # responds to a thread on Slack, we just assume it's the assignee. assign_and_broadcast_conversation_updated(conversation, primary_user_id) - %{ + response + |> Slack.Extractor.extract_slack_conversation_thread_info!() + |> Map.merge(%{ conversation_id: conversation_id, account_id: conversation.account_id - } - |> Map.merge(extract_slack_conversation_thread_info(response)) + }) |> SlackConversationThreads.create_slack_conversation_thread() end end @@ -301,7 +532,7 @@ defmodule ChatApi.Slack.Helpers do end end - @spec get_conversation_primary_user_id(Conversations.Conversation.t()) :: binary() + @spec get_conversation_primary_user_id(Conversation.t()) :: binary() def get_conversation_primary_user_id(conversation) do # TODO: do a round robin here instead of just getting the first user every time? conversation @@ -327,9 +558,26 @@ defmodule ChatApi.Slack.Helpers do def get_message_type(%Message{user_id: nil}), do: :customer def get_message_type(_message), do: :unknown + @spec is_bot_message?(map()) :: boolean() def is_bot_message?(%{"bot_id" => bot_id}) when not is_nil(bot_id), do: true def is_bot_message?(_), do: false + @spec is_agent_message?(SlackAuthorization.t(), map()) :: boolean() + def is_agent_message?(authorization, %{"user" => slack_user_id}) + when not is_nil(slack_user_id) do + case find_matching_user(authorization, slack_user_id) do + %User{} -> true + _ -> false + end + end + + def is_agent_message?(_authorization, _), do: false + + @spec is_customer_message?(SlackAuthorization.t(), map()) :: boolean() + def is_customer_message?(authorization, slack_message) do + !is_bot_message?(slack_message) && !is_agent_message?(authorization, slack_message) + end + @spec sanitize_slack_message(binary(), SlackAuthorization.t()) :: binary() def sanitize_slack_message(text, %SlackAuthorization{ access_token: access_token @@ -447,7 +695,9 @@ defmodule ChatApi.Slack.Helpers do end end - @spec slack_ts_to_utc(binary()) :: DateTime.t() + @spec slack_ts_to_utc(binary() | nil) :: DateTime.t() + def slack_ts_to_utc(nil), do: DateTime.utc_now() + def slack_ts_to_utc(ts) do with {unix, _} <- Float.parse(ts), microseconds <- round(unix * 1_000_000), @@ -458,100 +708,6 @@ defmodule ChatApi.Slack.Helpers do end end - ##################### - # Extractors - ##################### - - @spec extract_slack_message(map()) :: {:ok, map()} | {:error, String.t()} - def extract_slack_message(%{body: %{"ok" => true, "messages" => [message | _]}}), - do: {:ok, message} - - def extract_slack_message(%{body: %{"ok" => true, "messages" => []}}), - do: {:error, "No messages were found"} - - def extract_slack_message(%{body: %{"ok" => false} = body}) do - Logger.error("conversations.history returned ok=false: #{inspect(body)}") - - {:error, "conversations.history returned ok=false: #{inspect(body)}"} - end - - def extract_slack_message(response), - do: {:error, "Invalid response: #{inspect(response)}"} - - @spec extract_slack_messages(map()) :: {:ok, [map()]} | {:error, String.t()} - def extract_slack_messages(%{body: %{"ok" => true, "messages" => messages}}) - when is_list(messages), - do: {:ok, messages} - - def extract_slack_messages(%{body: %{"ok" => false} = body}) do - Logger.error("conversations.replies returned ok=false: #{inspect(body)}") - - {:error, "conversations.replies returned ok=false: #{inspect(body)}"} - end - - def extract_slack_messages(response), - do: {:error, "Invalid response: #{inspect(response)}"} - - @spec extract_slack_channel(map()) :: {:ok, map()} | {:error, String.t()} - def extract_slack_channel(%{body: %{"ok" => true, "channel" => channel}}) when is_map(channel), - do: {:ok, channel} - - def extract_slack_channel(%{body: %{"ok" => false} = body}) do - Logger.error("conversations.info returned ok=false: #{inspect(body)}") - - {:error, "conversations.info returned ok=false: #{inspect(body)}"} - end - - def extract_slack_channel(response), - do: {:error, "Invalid response: #{inspect(response)}"} - - @slackbot_user_id "USLACKBOT" - - @spec extract_valid_slack_users(map()) :: {:ok, [map()]} | {:error, String.t()} - def extract_valid_slack_users(%{body: %{"ok" => true, "members" => members}}) do - users = - Enum.reject(members, fn member -> - Map.get(member, "is_bot") || - Map.get(member, "deleted") || - member["id"] == @slackbot_user_id - end) - - {:ok, users} - end - - def extract_valid_slack_users(%{body: %{"ok" => true, "members" => []}}), - do: {:error, "No users were found"} - - def extract_valid_slack_users(response), - do: {:error, "Invalid response: #{inspect(response)}"} - - # TODO: refactor extractors below to return :ok/:error tuples rather than raising? - - @spec extract_slack_conversation_thread_info(map()) :: map() - def extract_slack_conversation_thread_info(%{body: body}) do - if Map.get(body, "ok") do - %{ - slack_channel: Map.get(body, "channel"), - slack_thread_ts: Map.get(body, "ts") - } - else - Logger.error("Error sending Slack message: #{inspect(body)}") - - raise "chat.postMessage returned ok=false" - end - end - - @spec extract_slack_user_email(map()) :: binary() - def extract_slack_user_email(%{body: body}) do - if Map.get(body, "ok") do - get_in(body, ["user", "profile", "email"]) - else - Logger.error("Error retrieving user info: #{inspect(body)}") - - raise "users.info returned ok=false" - end - end - ##################### # Formatters ##################### @@ -575,6 +731,7 @@ defmodule ChatApi.Slack.Helpers do url = base <> "/conversations/all?cid=" <> conversation_id dashboard = "<#{url}|dashboard>" + # 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:" end diff --git a/lib/chat_api/slack/notification.ex b/lib/chat_api/slack/notification.ex index 54d407bc1..df9e86488 100644 --- a/lib/chat_api/slack/notification.ex +++ b/lib/chat_api/slack/notification.ex @@ -65,14 +65,12 @@ defmodule ChatApi.Slack.Notification do thread: thread } |> Slack.Helpers.get_message_text() - |> IO.inspect(label: "message text") |> Slack.Helpers.get_message_payload(%{ channel: channel, customer: customer, thread: thread, message: message }) - |> IO.inspect(label: "message payload") |> Slack.Client.send_message(access_token) |> case do # Just pass through in test/dev mode (not sure if there's a more idiomatic way to do this) diff --git a/lib/chat_api/slack/sync.ex b/lib/chat_api/slack/sync.ex new file mode 100644 index 000000000..ed3f63e05 --- /dev/null +++ b/lib/chat_api/slack/sync.ex @@ -0,0 +1,187 @@ +defmodule ChatApi.Slack.Sync do + require Logger + + alias ChatApi.{ + Conversations, + Messages, + Slack, + SlackAuthorizations, + SlackConversationThreads + } + + alias ChatApi.Customers.Customer + alias ChatApi.SlackAuthorizations.SlackAuthorization + alias ChatApi.Users.User + + defmodule SyncableMessageInfo do + defstruct [:message, :sender, :is_bot] + + @type t :: %__MODULE__{ + message: map(), + sender: Customer.t() | User.t() | nil, + is_bot: boolean() + } + end + + @spec get_syncable_slack_messages(map()) :: [SyncableMessageInfo.t()] + def get_syncable_slack_messages( + %{ + "type" => "message", + "team" => team, + "text" => _text, + "thread_ts" => _thread_ts, + "channel" => _slack_channel_id + } = event + ) do + with %SlackAuthorization{access_token: _access_token} = authorization <- + SlackAuthorizations.find_slack_authorization(%{ + team_id: team, + type: "support" + }) do + get_syncable_slack_messages(authorization, event) + else + _ -> [] + end + end + + @spec get_syncable_slack_messages(any(), map()) :: [SyncableMessageInfo.t()] + def get_syncable_slack_messages( + %SlackAuthorization{access_token: access_token} = authorization, + %{ + "type" => "message", + "text" => _text, + "team" => _team, + "thread_ts" => thread_ts, + "channel" => slack_channel_id + } = _event + ) do + with :ok <- Slack.Validation.validate_channel_supported(authorization, slack_channel_id), + {:ok, response} <- + Slack.Client.retrieve_conversation_replies(slack_channel_id, thread_ts, access_token), + {:ok, slack_messages} <- Slack.Extractor.extract_slack_messages(response) do + Enum.map(slack_messages, fn msg -> + %SyncableMessageInfo{ + message: msg, + sender: Slack.Helpers.get_sender_info(authorization, msg), + is_bot: Slack.Helpers.is_bot_message?(msg) + } + end) + else + _ -> [] + end + end + + @spec should_sync_slack_messages?([SyncableMessageInfo.t()]) :: boolean() + def should_sync_slack_messages?([initial | replies]) do + is_valid_initial = + case initial do + %{is_bot: true} -> true + %{sender: %User{}} -> true + _ -> false + end + + has_customer_reply = + Enum.any?(replies, fn reply -> + case reply do + %{is_bot: false, sender: %Customer{}} -> true + _ -> false + end + end) + + is_valid_initial && has_customer_reply + end + + # TODO: do a better job distinguishing between Slack webhook event and Slack message payload + @spec sync_slack_message_thread([SyncableMessageInfo.t()], SlackAuthorization.t(), map()) :: + any() + def sync_slack_message_thread( + syncable_message_items, + %SlackAuthorization{account_id: account_id} = authorization, + %{ + "type" => "message", + "thread_ts" => thread_ts, + "channel" => slack_channel_id + } = _event + ) do + # TODO: make it possible to pass in customer manually + with %{sender: %Customer{} = customer} <- + Enum.find(syncable_message_items, fn item -> + case item do + %{is_bot: false, sender: %Customer{}} -> true + _ -> false + end + end), + {:ok, conversation} <- + Conversations.create_conversation(%{ + account_id: account_id, + customer_id: customer.id, + source: "slack" + }), + {:ok, _slack_conversation_thread} <- + SlackConversationThreads.create_slack_conversation_thread(%{ + slack_channel: slack_channel_id, + slack_thread_ts: thread_ts, + account_id: account_id, + conversation_id: conversation.id + }) do + conversation + |> Conversations.Notification.broadcast_conversation_to_admin!() + |> Conversations.Notification.broadcast_conversation_to_customer!() + + Enum.map(syncable_message_items, fn + %{ + message: %{"text" => text} = message, + sender: sender + } -> + sender = + case sender do + nil -> create_or_update_sender!(message, authorization) + sender -> sender + end + + message_sender_params = + case sender do + %User{id: user_id} -> %{"user_id" => user_id} + %Customer{id: customer_id} -> %{"customer_id" => customer_id} + # TODO: if no sender exists yet, create one! + _ -> raise "Unexpected sender #{inspect(sender)}" + end + + %{ + "body" => Slack.Helpers.sanitize_slack_message(text, authorization), + "conversation_id" => conversation.id, + "account_id" => account_id, + "sent_at" => message |> Map.get("ts") |> Slack.Helpers.slack_ts_to_utc(), + "source" => "slack" + } + |> Map.merge(message_sender_params) + |> Messages.create_and_fetch!() + |> Messages.Notification.broadcast_to_customer!() + |> Messages.Notification.broadcast_to_admin!() + |> Messages.Notification.notify(:webhooks) + # NB: we need to make sure the messages are created in the correct order, so we set async: false + |> Messages.Notification.notify(:slack, async: false) + # TODO: not sure we need to do this on every message + |> Messages.Helpers.handle_post_creation_conversation_updates() + + _ -> + nil + end) + end + end + + @spec create_or_update_sender!(map(), SlackAuthorization.t()) :: Customer.t() + def create_or_update_sender!(%{"user" => slack_user_id}, authorization) do + case Slack.Helpers.create_or_update_customer_from_slack_user_id(authorization, slack_user_id) do + {:ok, customer} -> customer + error -> raise "Failed to create customer from Slack user token: #{inspect(error)}" + end + end + + def create_or_update_sender!(%{"bot_id" => slack_bot_id}, authorization) do + case Slack.Helpers.create_or_update_customer_from_slack_bot_id(authorization, slack_bot_id) do + {:ok, customer} -> customer + error -> raise "Failed to create customer from Slack bot token: #{inspect(error)}" + end + end +end diff --git a/lib/chat_api/slack/validation.ex b/lib/chat_api/slack/validation.ex new file mode 100644 index 000000000..add46fd87 --- /dev/null +++ b/lib/chat_api/slack/validation.ex @@ -0,0 +1,54 @@ +defmodule ChatApi.Slack.Validation do + require Logger + + alias ChatApi.{ + Companies, + Slack, + SlackConversationThreads + } + + alias ChatApi.SlackAuthorizations.SlackAuthorization + + @spec validate_non_admin_user(any(), binary()) :: :ok | :error + def validate_non_admin_user(authorization, slack_user_id) do + case Slack.Helpers.find_matching_user(authorization, slack_user_id) do + nil -> :ok + _match -> :error + end + end + + @spec validate_channel_supported(any(), binary()) :: :ok | :error + def validate_channel_supported( + %SlackAuthorization{channel_id: slack_channel_id}, + slack_channel_id + ), + do: :ok + + def validate_channel_supported( + %SlackAuthorization{account_id: account_id}, + slack_channel_id + ) do + case Companies.find_by_slack_channel(account_id, slack_channel_id) do + nil -> :error + _company -> :ok + end + end + + def validate_channel_supported(_authorization, _slack_channel_id), do: :error + + @spec validate_no_existing_company(binary(), binary()) :: :ok | :error + def validate_no_existing_company(account_id, slack_channel_id) do + case Companies.find_by_slack_channel(account_id, slack_channel_id) do + nil -> :ok + _company -> :error + end + end + + @spec validate_no_existing_thread(binary(), binary()) :: :ok | :error + def validate_no_existing_thread(channel, ts) do + case SlackConversationThreads.exists?(%{"slack_thread_ts" => ts, "slack_channel" => channel}) do + false -> :ok + true -> :error + end + end +end diff --git a/lib/mix/tasks/set_missing_slack_user_ids.ex b/lib/mix/tasks/set_missing_slack_user_ids.ex index 4d910ff82..c7094ef3c 100644 --- a/lib/mix/tasks/set_missing_slack_user_ids.ex +++ b/lib/mix/tasks/set_missing_slack_user_ids.ex @@ -94,7 +94,7 @@ defmodule Mix.Tasks.SetMissingSlackUserIds do defp retrieve_slack_users(access_token) do with {:ok, response} <- ChatApi.Slack.Client.list_users(access_token), - {:ok, users} <- ChatApi.Slack.Helpers.extract_valid_slack_users(response) do + {:ok, users} <- ChatApi.Slack.Extractor.extract_valid_slack_users(response) do users else error -> diff --git a/test/chat_api/customers_test.exs b/test/chat_api/customers_test.exs index df8001b35..7a6853084 100644 --- a/test/chat_api/customers_test.exs +++ b/test/chat_api/customers_test.exs @@ -306,5 +306,60 @@ defmodule ChatApi.CustomersTest do assert {:error, _error} = Customers.create_or_update_by_email(nil, account.id, %{name: "New Customer"}) end + + test "create_or_update_by_external_id/3 finds the matching customer", %{account: account} do + external_id = "a0xxxxxxx1yz" + %{id: customer_id} = insert(:customer, %{external_id: external_id, account: account}) + + assert {:ok, %Customer{id: ^customer_id}} = + Customers.create_or_update_by_external_id(external_id, account.id) + end + + test "create_or_update_by_external_id/3 updates the matching customer", %{account: account} do + external_id = "a0xxxxxxx1yz" + name = "Test User" + %{id: customer_id} = insert(:customer, %{external_id: external_id, account: account}) + + assert {:ok, %Customer{id: ^customer_id, name: ^name}} = + Customers.create_or_update_by_external_id(external_id, account.id, %{name: name}) + end + + test "create_or_update_by_external_id/3 creates a new customer if necessary", %{ + account: account + } do + external_id = "a0xxxxxxx1yz" + %{id: customer_id} = insert(:customer, %{external_id: "other@test.com", account: account}) + + assert {:ok, %Customer{} = customer} = + Customers.create_or_update_by_external_id(external_id, account.id) + + assert customer.id != customer_id + assert customer.external_id == external_id + end + + test "create_or_update_by_external_id/3 creates a new customer with additional params", %{ + account: account + } do + external_id = "a0xxxxxxx1yz" + %{id: customer_id} = insert(:customer, %{email: "other@test.com", account: account}) + + assert {:ok, %Customer{} = customer} = + Customers.create_or_update_by_external_id(external_id, account.id, %{ + name: "New Customer" + }) + + assert customer.id != customer_id + assert customer.external_id == external_id + assert customer.name == "New Customer" + end + + test "create_or_update_by_external_id/3 returns an :error tuple if email is nil", %{ + account: account + } do + assert {:error, _error} = Customers.create_or_update_by_external_id(nil, account.id) + + assert {:error, _error} = + Customers.create_or_update_by_external_id(nil, account.id, %{name: "New Customer"}) + end end end diff --git a/test/chat_api/slack_test.exs b/test/chat_api/slack_test.exs index 857e79cbd..1434619e0 100644 --- a/test/chat_api/slack_test.exs +++ b/test/chat_api/slack_test.exs @@ -379,38 +379,38 @@ defmodule ChatApi.SlackTest do }) end - test "Helpers.extract_slack_conversation_thread_info/1 extracts thread info from slack response" do + test "Extractor.extract_slack_conversation_thread_info!/1 extracts thread info from slack response" do channel = "bots" ts = "1234.56789" response = %{body: %{"ok" => true, "channel" => channel, "ts" => ts}} assert %{slack_channel: ^channel, slack_thread_ts: ^ts} = - Slack.Helpers.extract_slack_conversation_thread_info(response) + Slack.Extractor.extract_slack_conversation_thread_info!(response) end - test "Helpers.extract_slack_conversation_thread_info/1 raises if the slack response has ok=false" do + test "Extractor.extract_slack_conversation_thread_info!/1 raises if the slack response has ok=false" do response = %{body: %{"ok" => false}} assert capture_log(fn -> assert_raise RuntimeError, fn -> - Slack.Helpers.extract_slack_conversation_thread_info(response) + Slack.Extractor.extract_slack_conversation_thread_info!(response) end end) =~ "Error sending Slack message" end - test "Helpers.extract_slack_user_email/1 extracts user's email from slack response" do + test "Extractor.extract_slack_user_email!/1 extracts user's email from slack response" do email = "test@test.com" response = %{body: %{"ok" => true, "user" => %{"profile" => %{"email" => email}}}} - assert email = Slack.Helpers.extract_slack_user_email(response) + assert email = Slack.Extractor.extract_slack_user_email!(response) end - test "Helpers.extract_slack_user_email/1 raises if the slack response has ok=false" do + test "Extractor.extract_slack_user_email!/1 raises if the slack response has ok=false" do response = %{body: %{"ok" => false, "user" => nil}} assert capture_log(fn -> assert_raise RuntimeError, fn -> - Slack.Helpers.extract_slack_user_email(response) + Slack.Extractor.extract_slack_user_email!(response) end end) =~ "Error retrieving user info" end @@ -581,6 +581,113 @@ defmodule ChatApi.SlackTest do end end + test "Helpers.format_sender_id_v2!/3 gets an existing user_id", %{account: account} do + authorization = insert(:slack_authorization, account: account) + _customer = insert(:customer, account: account, email: "customer@customer.com") + user = insert(:user, account: account, email: "user@user.com") + + slack_user = %{ + "real_name" => "Test User", + "tz" => "America/New_York", + "profile" => %{"email" => "user@user.com", "real_name" => "Test User"} + } + + slack_event = %{ + "type" => "message", + "channel" => @slack_channel_id, + "user" => @slack_user_id + } + + with_mock ChatApi.Slack.Client, + retrieve_user_info: fn _, _ -> + {:ok, %{body: %{"ok" => true, "user" => slack_user}}} + end do + refute Slack.Helpers.find_matching_customer(authorization, @slack_user_id) + assert %{id: user_id} = Slack.Helpers.find_matching_user(authorization, @slack_user_id) + assert user_id == user.id + + assert %{"user_id" => ^user_id} = + Slack.Helpers.format_sender_id_v2!(authorization, slack_event) + end + end + + test "Helpers.format_sender_id_v2!/3 gets an existing customer_id", %{account: account} do + authorization = insert(:slack_authorization, account: account) + customer = insert(:customer, account: account, email: "customer@customer.com") + _user = insert(:user, account: account, email: "user@user.com") + + slack_user = %{ + "real_name" => "Test Customer", + "tz" => "America/New_York", + "profile" => %{"email" => "customer@customer.com", "real_name" => "Test Customer"} + } + + slack_event = %{ + "type" => "message", + "channel" => @slack_channel_id, + "user" => @slack_user_id + } + + with_mock ChatApi.Slack.Client, + retrieve_user_info: fn _, _ -> + {:ok, %{body: %{"ok" => true, "user" => slack_user}}} + end do + assert %{id: customer_id} = + Slack.Helpers.find_matching_customer(authorization, @slack_user_id) + + refute Slack.Helpers.find_matching_user(authorization, @slack_user_id) + assert customer_id == customer.id + + assert %{"customer_id" => ^customer_id} = + Slack.Helpers.format_sender_id_v2!(authorization, slack_event) + end + end + + test "Helpers.format_sender_id_v2!/3 creates a new customer_id if necessary", %{ + account: account + } do + authorization = insert(:slack_authorization, account: account) + _customer = insert(:customer, account: account, email: "customer@customer.com") + _user = insert(:user, account: account, email: "user@user.com") + + company = + insert(:company, + account: account, + name: "Slack Test Co", + slack_channel_id: @slack_channel_id + ) + + slack_user = %{ + "real_name" => "Test Customer", + "tz" => "America/New_York", + # New customer email + "profile" => %{"email" => "new@customer.com", "real_name" => "Test Customer"} + } + + slack_event = %{ + "type" => "message", + "channel" => @slack_channel_id, + "user" => @slack_user_id + } + + with_mock ChatApi.Slack.Client, + retrieve_user_info: fn _, _ -> + {:ok, %{body: %{"ok" => true, "user" => slack_user}}} + end do + refute Slack.Helpers.find_matching_customer(authorization, @slack_user_id) + refute Slack.Helpers.find_matching_user(authorization, @slack_user_id) + + assert %{"customer_id" => customer_id} = + Slack.Helpers.format_sender_id_v2!(authorization, slack_event) + + customer = ChatApi.Customers.get_customer!(customer_id) + + assert customer.email == "new@customer.com" + assert customer.name == "Test Customer" + assert customer.company_id == company.id + end + end + test "Helpers.create_or_update_customer_from_slack_user_id/3 creates or updates the customer", %{account: account} do authorization = insert(:slack_authorization, account: account) diff --git a/test/chat_api_web/controllers/slack_controller_test.exs b/test/chat_api_web/controllers/slack_controller_test.exs index 80f5429c4..8f9b238c4 100644 --- a/test/chat_api_web/controllers/slack_controller_test.exs +++ b/test/chat_api_web/controllers/slack_controller_test.exs @@ -275,15 +275,24 @@ defmodule ChatApiWeb.SlackControllerTest do "profile" => %{"email" => @email} } + slack_bot_user = %{ + "id" => "B123", + "name" => "Papercups Bot" + } + slack_bot_message = %{ "text" => "This is a bot message", - "bot_id" => "B123" + "bot_id" => "B123", + "ts" => event_params["thread_ts"] } with_mock ChatApi.Slack.Client, retrieve_user_info: fn _, _ -> {:ok, %{body: %{"ok" => true, "user" => slack_user}}} end, + retrieve_bot_info: fn _, _ -> + {:ok, %{body: %{"ok" => true, "bot" => slack_bot_user}}} + end, retrieve_message: fn _, _, _ -> {:ok, %{body: %{"ok" => true, "messages" => [slack_bot_message]}}} end, @@ -328,8 +337,7 @@ defmodule ChatApiWeb.SlackControllerTest do slack_bot_message = %{ "text" => "This is a non-bot message", - "user" => "U123TEST", - "bot_id" => nil + "user" => "U123TEST" } with_mock ChatApi.Slack.Client,