Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #156 - Deprecate "shout" event #488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/chat_api/messages/notification.ex
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ defmodule ChatApi.Messages.Notification do
})
end

@spec broadcast_to_conversation!(Message.t()) :: Message.t()
def broadcast_to_conversation!(%Message{} = message) do
@spec broadcast_to_conversation!(Message.t(), String.t()) :: Message.t()
def broadcast_to_conversation!(%Message{} = message, event_name \\ "shout") do
message
|> Helpers.get_conversation_topic()
|> ChatApiWeb.Endpoint.broadcast!("shout", Helpers.format(message))
|> ChatApiWeb.Endpoint.broadcast!(event_name, Helpers.format(message))

message
end
Expand Down
46 changes: 31 additions & 15 deletions lib/chat_api_web/channels/conversation_channel.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ defmodule ChatApiWeb.ConversationChannel do

alias ChatApiWeb.Presence
alias ChatApi.{Messages, Conversations}
require Logger

@impl true
def join("conversation:lobby", payload, socket) do
Expand Down Expand Up @@ -79,20 +80,16 @@ defmodule ChatApiWeb.ConversationChannel do
end

def handle_in("shout", payload, socket) do
with %{conversation: conversation} <- socket.assigns,
%{id: conversation_id, account_id: account_id} <- conversation,
{:ok, message} <-
payload
|> Map.merge(%{"conversation_id" => conversation_id, "account_id" => account_id})
|> Messages.create_message(),
message <- Messages.get_message!(message.id) do
broadcast_new_message(socket, message)
else
_ ->
broadcast(socket, "shout", payload)
end
Logger.warn(
"'shout' is deprecated as event name on a new message and will be removed in a future version. Please migrate to a newer version of a client."
)

{:noreply, socket}
handle_incoming_message("shout", payload, socket)
end

@impl true
def handle_in("message:created", payload, socket) do
handle_incoming_message("message:created", payload, socket)
end

def handle_in("messages:seen", _payload, socket) do
Expand All @@ -119,9 +116,9 @@ defmodule ChatApiWeb.ConversationChannel do
})
end

defp broadcast_new_message(socket, message) do
defp broadcast_new_message(socket, event_name, message) do
broadcast_conversation_update(message)
broadcast(socket, "shout", Messages.Helpers.format(message))
broadcast(socket, event_name, Messages.Helpers.format(message))

message
|> Messages.Notification.notify(:slack)
Expand All @@ -143,4 +140,23 @@ defmodule ChatApiWeb.ConversationChannel do
_ -> false
end
end

# It is also common to receive messages from the client and
# broadcast to everyone in the current topic (conversation:lobby).
defp handle_incoming_message(event_name, payload, socket) do
with %{conversation: conversation} <- socket.assigns,
%{id: conversation_id, account_id: account_id} <- conversation,
{:ok, message} <-
payload
|> Map.merge(%{"conversation_id" => conversation_id, "account_id" => account_id})
|> Messages.create_message(),
message <- Messages.get_message!(message.id) do
broadcast_new_message(socket, event_name, message)
else
_ ->
broadcast(socket, event_name, payload)
end

{:noreply, socket}
end
end
43 changes: 28 additions & 15 deletions lib/chat_api_web/channels/notification_channel.ex
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,16 @@ defmodule ChatApiWeb.NotificationChannel do
end

def handle_in("shout", payload, socket) do
with %{current_user: current_user} <- socket.assigns,
%{id: user_id, account_id: account_id} <- current_user do
{:ok, message} =
payload
|> Map.merge(%{"user_id" => user_id, "account_id" => account_id})
|> Messages.create_message()
Logger.warn(
"'shout' is deprecated as event name on a new message and will be removed in a future version. Please migrate to a newer version of a client."
)

message
|> Map.get(:id)
|> Messages.get_message!()
|> broadcast_new_message()
|> Messages.Helpers.handle_post_creation_conversation_updates()
end
handle_incoming_message("shout", payload, socket)
end

{:reply, :ok, socket}
@impl true
def handle_in("message:created", payload, socket) do
handle_incoming_message("message:created", payload, socket)
end

@impl true
Expand Down Expand Up @@ -105,9 +100,9 @@ defmodule ChatApiWeb.NotificationChannel do
{:noreply, socket}
end

defp broadcast_new_message(message) do
defp broadcast_new_message(message, event_name) do
message
|> Messages.Notification.broadcast_to_conversation!()
|> Messages.Notification.broadcast_to_conversation!(event_name)
|> Messages.Notification.notify(:slack)
|> Messages.Notification.notify(:slack_support_channel)
|> Messages.Notification.notify(:slack_company_channel)
Expand Down Expand Up @@ -137,4 +132,22 @@ defmodule ChatApiWeb.NotificationChannel do
_ -> false
end
end

defp handle_incoming_message(event_name, payload, socket) do
with %{current_user: current_user} <- socket.assigns,
%{id: user_id, account_id: account_id} <- current_user do
{:ok, message} =
payload
|> Map.merge(%{"user_id" => user_id, "account_id" => account_id})
|> Messages.create_message()

message
|> Map.get(:id)
|> Messages.get_message!()
|> broadcast_new_message(event_name)
|> Messages.Helpers.handle_post_creation_conversation_updates()
end

{:reply, :ok, socket}
end
end
6 changes: 6 additions & 0 deletions test/chat_api_web/channels/conversation_channel_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ defmodule ChatApiWeb.ConversationChannelTest do
assert_broadcast "shout", _msg
end

test "message:created broadcasts to conversation:lobby", %{socket: socket, account: account} do
msg = %{body: "Hello world!", account_id: account.id}
push(socket, "message:created", msg)
assert_broadcast "message:created", _msg
end

test "broadcasts are pushed to the client", %{socket: socket} do
broadcast_from!(socket, "broadcast", %{"some" => "data"})
assert_push "broadcast", %{"some" => "data"}
Expand Down
16 changes: 16 additions & 0 deletions test/chat_api_web/channels/notification_channel_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,22 @@ defmodule ChatApiWeb.NotificationChannelTest do
assert_push("shout", _msg)
end

test "message:created broadcasts to notification:lobby", %{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this test is failing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

socket: socket,
account: account,
conversation: conversation
} do
msg = %{
body: "Message created!",
account_id: account.id,
conversation_id: conversation.id
}

push(socket, "message:created", msg)

assert_push("message:created", _msg)
end

test "broadcasts are pushed to the client", %{socket: socket} do
broadcast_from!(socket, "broadcast", %{"some" => "data"})
assert_push "broadcast", %{"some" => "data"}
Expand Down