diff --git a/README.md b/README.md index a704263..97aef27 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ See [open issues](https://github.com/joebew42/ex_chat/issues) for the full list - `bar_user` associated to the token `bar_token` - Maybe the `AuthenticationService` is a "Repository" instead. Consider to rename it - Provide a real implementation of the `AuthenticationService` -- Extract a collaborator for the `WebSocketClient` that will be responsible to understand if there is an existing user_session for a given access_token +- Extract a collaborator for the `WebSocketClient` that will be responsible to understand if there is an existing user_id for a given access_token - Update the UI in order to handle the user id - It seems that we have a [websocket idle timeout issue](https://ninenines.eu/docs/en/cowboy/2.4/guide/ws_handlers/#_keeping_the_connection_alive). Increase the idle timeout to 10 minutes - Handle the connection when the provided access token is empty or not valid (no user session associated) @@ -83,9 +83,9 @@ See [open issues](https://github.com/joebew42/ex_chat/issues) for the full list - The module `ChatRooms` should be reorganized like the `UserSessions` - As a `ChatRoom` I can notify of new messages to all the subscribed `UserSession`s - rename the `UserSessions.send` to `UserSessions.notify` -- think to rename `clients` to `session_ids` in the `ChatRoom` process +- think to rename `clients` to `user_ids` in the `ChatRoom` process - Rename `ExChat.Registry` in `ExChat.ChatRoomRegistry` -- rename `user_session_id` to `session_id` +- rename `user_session_id` to `user_id` - Maybe the `UserSessions` and `UserSessionSupervisor` could be merged in a single module named `UserSessions` - Fix the names used for the user sessions in the `UserSessionsTest` - Try to find a way to remove the shared state (the `UserSessionRegistry`) from the `UserSessions` Tests diff --git a/lib/ex_chat/access_token_repository.ex b/lib/ex_chat/access_token_repository.ex index 0f33229..f8f477d 100644 --- a/lib/ex_chat/access_token_repository.ex +++ b/lib/ex_chat/access_token_repository.ex @@ -5,8 +5,8 @@ defmodule ExChat.AccessTokenRepository do # Client API # ############## - def add(access_token, user_session) do - :ok = GenServer.call(:access_token_repository, {:add, access_token, user_session}) + def add(access_token, user_id) do + :ok = GenServer.call(:access_token_repository, {:add, access_token, user_id}) end def find_user_session_by(access_token) do @@ -25,8 +25,8 @@ defmodule ExChat.AccessTokenRepository do {:ok, %{}} end - def handle_call({:add, access_token, user_session}, _from, state) do - {:reply, :ok, Map.put(state, access_token, user_session)} + def handle_call({:add, access_token, user_id}, _from, state) do + {:reply, :ok, Map.put(state, access_token, user_id)} end def handle_call({:find_user_session_by, access_token}, _from, state) do diff --git a/lib/ex_chat/chat_room.ex b/lib/ex_chat/chat_room.ex index bde6f6e..8762202 100644 --- a/lib/ex_chat/chat_room.ex +++ b/lib/ex_chat/chat_room.ex @@ -3,7 +3,7 @@ defmodule ExChat.ChatRoom do alias ExChat.UserSessions - defstruct session_ids: [], name: nil + defstruct user_ids: [], name: nil def create(name = {:via, Registry, {_registry_name, chatroom_name}}) do GenServer.start_link(__MODULE__, %__MODULE__{name: chatroom_name}, name: name) @@ -18,31 +18,31 @@ defmodule ExChat.ChatRoom do {:ok, state} end - def join(pid, session_id) do - GenServer.call(pid, {:join, session_id}) + def join(pid, user_id) do + GenServer.call(pid, {:join, user_id}) end - def send(pid, message, [as: session_id]) do - :ok = GenServer.call(pid, {:send, message, :as, session_id}) + def send(pid, message, [as: user_id]) do + :ok = GenServer.call(pid, {:send, message, :as, user_id}) end - def handle_call({:join, session_id}, _from, state) do - {message, new_state} = case joined?(state.session_ids, session_id) do + def handle_call({:join, user_id}, _from, state) do + {message, new_state} = case joined?(state.user_ids, user_id) do true -> {{:error, :already_joined}, state} - false -> {:ok, add_session_id(state, session_id)} + false -> {:ok, add_user_id(state, user_id)} end {:reply, message, new_state} end - def handle_call({:send, message, :as, session_id}, _from, state = %__MODULE__{name: name}) do - Enum.each(state.session_ids, &UserSessions.notify(%{from: session_id, room: name, message: message}, to: &1)) + def handle_call({:send, message, :as, user_id}, _from, state = %__MODULE__{name: name}) do + Enum.each(state.user_ids, &UserSessions.notify(%{from: user_id, room: name, message: message}, to: &1)) {:reply, :ok, state} end - defp joined?(session_ids, session_id), do: Enum.member?(session_ids, session_id) + defp joined?(user_ids, user_id), do: Enum.member?(user_ids, user_id) - defp add_session_id(state = %__MODULE__{session_ids: session_ids}, session_id) do - %__MODULE__{state | session_ids: [session_id|session_ids]} + defp add_user_id(state = %__MODULE__{user_ids: user_ids}, user_id) do + %__MODULE__{state | user_ids: [user_id|user_ids]} end end diff --git a/lib/ex_chat/chat_rooms.ex b/lib/ex_chat/chat_rooms.ex index 9c4cd06..ada5918 100644 --- a/lib/ex_chat/chat_rooms.ex +++ b/lib/ex_chat/chat_rooms.ex @@ -17,24 +17,24 @@ defmodule ExChat.ChatRooms do end end - def join(room, [as: session_id]) do + def join(room, [as: user_id]) do case find(room) do {:ok, pid} -> - try_join_chatroom(pid, session_id) + try_join_chatroom(pid, user_id) {:error, :unexisting_room} -> {:error, :unexisting_room} end end - def send(message, [to: room, as: session_id]) do + def send(message, [to: room, as: user_id]) do case find(room) do - {:ok, pid} -> ChatRoom.send(pid, message, as: session_id) + {:ok, pid} -> ChatRoom.send(pid, message, as: user_id) error -> error end end - defp try_join_chatroom(chatroom_pid, session_id) do - case ChatRoom.join(chatroom_pid, session_id) do + defp try_join_chatroom(chatroom_pid, user_id) do + case ChatRoom.join(chatroom_pid, user_id) do :ok -> :ok {:error, :already_joined} -> diff --git a/lib/ex_chat/use_cases/validate_access_token.ex b/lib/ex_chat/use_cases/validate_access_token.ex index 4046c0c..6afa3f5 100644 --- a/lib/ex_chat/use_cases/validate_access_token.ex +++ b/lib/ex_chat/use_cases/validate_access_token.ex @@ -6,8 +6,8 @@ defmodule ExChat.UseCases.ValidateAccessToken do case AccessTokenRepository.find_user_session_by(access_token) do nil -> {:error, :access_token_not_valid} - user_session -> - {:ok, user_session} + user_id -> + {:ok, user_id} end end end \ No newline at end of file diff --git a/lib/ex_chat/user_sessions.ex b/lib/ex_chat/user_sessions.ex index 4fbc6a3..5c9e583 100644 --- a/lib/ex_chat/user_sessions.ex +++ b/lib/ex_chat/user_sessions.ex @@ -7,25 +7,25 @@ defmodule ExChat.UserSessions do # Client API # ############## - def create(session_id) do - case find(session_id) do + def create(user_id) do + case find(user_id) do nil -> - start(session_id) + start(user_id) :ok _pid -> {:error, :already_exists} end end - def subscribe(client_pid, [to: session_id]) do - case find(session_id) do + def subscribe(client_pid, [to: user_id]) do + case find(user_id) do nil -> {:error, :session_not_exists} pid -> UserSession.subscribe(pid, client_pid) end end - def notify(message, [to: session_id]) do - case find(session_id) do + def notify(message, [to: user_id]) do + case find(user_id) do nil -> {:error, :session_not_exists} pid -> UserSession.notify(pid, message) end @@ -43,14 +43,14 @@ defmodule ExChat.UserSessions do DynamicSupervisor.init(strategy: :one_for_one) end - defp start(session_id) do - name = {:via, Registry, {UserSessionRegistry, session_id}} + defp start(user_id) do + name = {:via, Registry, {UserSessionRegistry, user_id}} DynamicSupervisor.start_child(__MODULE__, {UserSession ,name}) end - defp find(session_id) do - case Registry.lookup(UserSessionRegistry, session_id) do + defp find(user_id) do + case Registry.lookup(UserSessionRegistry, user_id) do [] -> nil [{pid, nil}] -> pid end diff --git a/lib/ex_chat/web/websocket_controller.ex b/lib/ex_chat/web/websocket_controller.ex index cf1a795..d3720cc 100644 --- a/lib/ex_chat/web/websocket_controller.ex +++ b/lib/ex_chat/web/websocket_controller.ex @@ -13,72 +13,72 @@ defmodule ExChat.Web.WebSocketController do access_token = access_token_from(req) case ValidateAccessToken.on(access_token) do - {:ok, user_session} -> - {:cowboy_websocket, req, user_session, %{idle_timeout: idle_timeout()}} + {:ok, user_id} -> + {:cowboy_websocket, req, user_id, %{idle_timeout: idle_timeout()}} {:error, :access_token_not_valid} -> {:ok, :cowboy_req.reply(400, req), state} end end - def websocket_init(user_session) do - SubscribeToUserSession.on(self(), user_session) + def websocket_init(user_id) do + SubscribeToUserSession.on(self(), user_id) schedule_ping() - {:ok, user_session} + {:ok, user_id} end - def websocket_handle({:text, command_as_json}, session_id) do + def websocket_handle({:text, command_as_json}, user_id) do case from_json(command_as_json) do - {:error, _reason} -> {:ok, session_id} - {:ok, command} -> handle(command, session_id) + {:error, _reason} -> {:ok, user_id} + {:ok, command} -> handle(command, user_id) end end - def websocket_handle(_message, session_id) do - {:ok, session_id} + def websocket_handle(_message, user_id) do + {:ok, user_id} end - def websocket_info(:ping, session_id) do + def websocket_info(:ping, user_id) do schedule_ping() - {:reply, {:ping, ""}, session_id} + {:reply, {:ping, ""}, user_id} end - def websocket_info(message, session_id) do - {:reply, {:text, to_json(message)}, session_id} + def websocket_info(message, user_id) do + {:reply, {:text, to_json(message)}, user_id} end - defp handle(%{"command" => "join", "room" => room}, session_id) do - case JoinChatRoom.on(room, session_id) do + defp handle(%{"command" => "join", "room" => room}, user_id) do + case JoinChatRoom.on(room, user_id) do :ok -> - {:ok, session_id} + {:ok, user_id} {:error, message} -> - {:reply, {:text, to_json(%{error: message})}, session_id} + {:reply, {:text, to_json(%{error: message})}, user_id} end end - defp handle(command = %{"command" => "join"}, session_id) do - handle(Map.put(command, "room", "default"), session_id) + defp handle(command = %{"command" => "join"}, user_id) do + handle(Map.put(command, "room", "default"), user_id) end - defp handle(%{"room" => room, "message" => message}, session_id) do - case SendMessageToChatRoom.on(message, room, session_id) do + defp handle(%{"room" => room, "message" => message}, user_id) do + case SendMessageToChatRoom.on(message, room, user_id) do {:error, message} -> - {:reply, {:text, to_json(%{ error: message })}, session_id} + {:reply, {:text, to_json(%{ error: message })}, user_id} :ok -> - {:ok, session_id} + {:ok, user_id} end end - defp handle(%{"command" => "create", "room" => room}, session_id) do + defp handle(%{"command" => "create", "room" => room}, user_id) do response = case CreateChatRoom.on(room) do {:ok, message} -> %{success: message} {:error, message} -> %{error: message} end - {:reply, {:text, to_json(response)}, session_id} + {:reply, {:text, to_json(response)}, user_id} end - defp handle(_not_handled_command, session_id), do: {:ok, session_id} + defp handle(_not_handled_command, user_id), do: {:ok, user_id} defp to_json(response), do: Poison.encode!(response) defp from_json(json), do: Poison.decode(json) diff --git a/test/ex_chat/access_token_repository_test.exs b/test/ex_chat/access_token_repository_test.exs index afae262..3b6df14 100644 --- a/test/ex_chat/access_token_repository_test.exs +++ b/test/ex_chat/access_token_repository_test.exs @@ -13,8 +13,8 @@ defmodule ExChat.AccessTokenRepositoryTest do end test "return the user session associated to the token" do - AccessTokenRepository.add("a-token", "a-user-session") + AccessTokenRepository.add("a-token", "a-user-id") - assert AccessTokenRepository.find_user_session_by("a-token") == "a-user-session" + assert AccessTokenRepository.find_user_session_by("a-token") == "a-user-id" end end \ No newline at end of file diff --git a/test/ex_chat/chat_room_test.exs b/test/ex_chat/chat_room_test.exs index 5a4354c..92a9fb4 100644 --- a/test/ex_chat/chat_room_test.exs +++ b/test/ex_chat/chat_room_test.exs @@ -10,7 +10,7 @@ defmodule ExChat.ChatRoomTest do {:ok, chatroom} = ChatRoom.create("room_name") ChatRoom.join(chatroom, "a-user-session-id") - with_mock UserSessions, [notify: fn(_message, [to: _user_session_id]) -> :ok end] do + with_mock UserSessions, [notify: fn(_message, [to: _user_id]) -> :ok end] do expected_message = %{ from: "another-user-session-id", room: "room_name", diff --git a/test/ex_chat/use_cases/validate_access_token_test.exs b/test/ex_chat/use_cases/validate_access_token_test.exs index 6c79351..6ba4be1 100644 --- a/test/ex_chat/use_cases/validate_access_token_test.exs +++ b/test/ex_chat/use_cases/validate_access_token_test.exs @@ -14,8 +14,8 @@ defmodule ExChat.UseCases.ValidateAccessTokenTest do end test "return the user session when there is an access token" do - with_mock(AccessTokenRepository, find_user_session_by: fn(_) -> "a user session" end) do - assert ValidateAccessToken.on("a valid token") == {:ok, "a user session"} + with_mock(AccessTokenRepository, find_user_session_by: fn(_) -> "a user id" end) do + assert ValidateAccessToken.on("a valid token") == {:ok, "a user id"} assert called AccessTokenRepository.find_user_session_by("a valid token") end end diff --git a/test/ex_chat/user_sessions_test.exs b/test/ex_chat/user_sessions_test.exs index 3e9b493..46d25e9 100644 --- a/test/ex_chat/user_sessions_test.exs +++ b/test/ex_chat/user_sessions_test.exs @@ -11,15 +11,15 @@ defmodule ExChat.UserSessionsTest do describe "when create a UserSession" do test "an error is received when the session already exist" do - UserSessions.create("a-session-id") + UserSessions.create("a-user-id") - result = UserSessions.create("a-session-id") + result = UserSessions.create("a-user-id") assert result == {:error, :already_exists} end test "an ok is received" do - result = UserSessions.create("a-session-id") + result = UserSessions.create("a-user-id") assert result == :ok end @@ -27,15 +27,15 @@ defmodule ExChat.UserSessionsTest do describe "when subscribe to a UserSession" do test "an error is received when the session does not exist" do - result = UserSessions.subscribe(self(), to: "a-session-id") + result = UserSessions.subscribe(self(), to: "a-user-id") assert result == {:error, :session_not_exists} end test "an ok is received when the session exists" do - UserSessions.create("a-session-id") + UserSessions.create("a-user-id") - result = UserSessions.subscribe(self(), to: "a-session-id") + result = UserSessions.subscribe(self(), to: "a-user-id") assert result == :ok end @@ -43,15 +43,15 @@ defmodule ExChat.UserSessionsTest do describe "when send a message to a UserSession" do test "an error is received when the session does not exist" do - result = UserSessions.notify("a message", to: "a-session-id") + result = UserSessions.notify("a message", to: "a-user-id") assert result == {:error, :session_not_exists} end test "a message is correctly delivered" do - UserSessions.create("a-session-id") + UserSessions.create("a-user-id") - result = UserSessions.notify("a message", to: "a-session-id") + result = UserSessions.notify("a message", to: "a-user-id") assert result == :ok end @@ -59,10 +59,10 @@ defmodule ExChat.UserSessionsTest do describe "when subscribed to a UserSession" do test "messages received are forwarded to subscribers" do - UserSessions.create("a-session-id") - UserSessions.subscribe(self(), to: "a-session-id") + UserSessions.create("a-user-id") + UserSessions.subscribe(self(), to: "a-user-id") - UserSessions.notify("a message", to: "a-session-id") + UserSessions.notify("a message", to: "a-user-id") assert_receive "a message" end @@ -70,9 +70,9 @@ defmodule ExChat.UserSessionsTest do describe "when not subscribed to a UserSession" do test "messages received are not forwarded" do - UserSessions.create("a-session-id") + UserSessions.create("a-user-id") - UserSessions.notify("a message", to: "a-session-id") + UserSessions.notify("a message", to: "a-user-id") refute_receive "a message" end