From aa23bf18d91763c2adf26d866d20b736855d3bae Mon Sep 17 00:00:00 2001
From: Erin Moore <erinmoorethefirst@gmail.com>
Date: Mon, 17 Jun 2024 13:49:38 -0400
Subject: [PATCH] PR feedback

---
 apps/alert_processor/lib/model/user.ex        |  5 +-
 .../test/alert_processor/model/user_test.exs  |  7 +-
 .../lib/controllers/auth_controller.ex        | 65 ++++++++-----------
 3 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/apps/alert_processor/lib/model/user.ex b/apps/alert_processor/lib/model/user.ex
index 754d0cb3f..406e12a4f 100644
--- a/apps/alert_processor/lib/model/user.ex
+++ b/apps/alert_processor/lib/model/user.ex
@@ -240,8 +240,11 @@ defmodule AlertProcessor.Model.User do
   @spec get(id()) :: t() | nil
   def get(id), do: Repo.get(__MODULE__, id)
 
-  @spec get_by_alternate_id(%{id: id() | nil, mbta_uuid: id() | nil}) :: [t()]
+  @spec get_by_alternate_id(%{id: id(), mbta_uuid: id() | nil}) :: [t()]
   def get_by_alternate_id(%{id: id, mbta_uuid: mbta_uuid}) do
+    # can get accounts using id or mbta_uuid
+    # which is the old-style ID for accounts from the time before SSO.
+
     from(u in __MODULE__,
       where: u.id in [^id, ^mbta_uuid]
     )
diff --git a/apps/alert_processor/test/alert_processor/model/user_test.exs b/apps/alert_processor/test/alert_processor/model/user_test.exs
index 837d3c736..4c443a981 100644
--- a/apps/alert_processor/test/alert_processor/model/user_test.exs
+++ b/apps/alert_processor/test/alert_processor/model/user_test.exs
@@ -215,7 +215,12 @@ defmodule AlertProcessor.Model.UserTest do
       assert User.get_by_alternate_id(%{id: nil, mbta_uuid: user.id}) == [user]
     end
 
-    test "returns two users if both present (unlikely)" do
+    test "returns a user if both id's are the same " do
+      user = insert(:user)
+      assert User.get_by_alternate_id(%{id: user.id, mbta_uuid: user.id}) == [user]
+    end
+
+    test "returns two users if both ids present and different (unlikely)" do
       user = insert(:user)
       user2 = insert(:user)
       assert User.get_by_alternate_id(%{id: user.id, mbta_uuid: user2.id}) == [user, user2]
diff --git a/apps/concierge_site/lib/controllers/auth_controller.ex b/apps/concierge_site/lib/controllers/auth_controller.ex
index 6eeaa4c7e..cba2a334e 100644
--- a/apps/concierge_site/lib/controllers/auth_controller.ex
+++ b/apps/concierge_site/lib/controllers/auth_controller.ex
@@ -32,11 +32,30 @@ defmodule ConciergeSite.AuthController do
     phone_number = PhoneNumber.strip_us_country_code(phone_number)
 
     role = parse_role({:ok, userinfo})
+    user_list = get_or_create_user(%{id: id, mbta_uuid: mbta_uuid})
 
     user =
-      %{id: id, mbta_uuid: mbta_uuid}
-      |> get_or_create_user(email, phone_number, role)
-      |> use_props_from_token(email, phone_number, role)
+      case length(user_list) do
+        0 ->
+          # If neither ID is found, the user just created their account in Keycloak so we need to add them to our database
+          Repo.insert!(%User{
+            id: id,
+            email: email,
+            phone_number: phone_number,
+            role: role
+          })
+
+          id |> User.get() |> use_props_from_token(email, phone_number, role)
+
+        1 ->
+          # If 1 user is found, we want to return that user
+          user_list |> hd |> use_props_from_token(email, phone_number, role)
+
+        2 ->
+          # If 2 users are found, something weird happened. Log and return nil. User will be redirected to landing page.
+          Logger.warn("User with 2 ids found. sub id: #{id}, mbta_uuid: #{mbta_uuid}")
+          nil
+      end
 
     logout_params = %{
       post_logout_redirect_uri: page_url(conn, :landing)
@@ -73,38 +92,11 @@ defmodule ConciergeSite.AuthController do
     SessionHelper.sign_out(conn)
   end
 
-  @spec get_or_create_user(
-          %{id: User.id() | nil, mbta_uuid: User.id() | nil},
-          String.t(),
-          String.t() | nil,
-          String.t()
-        ) ::
-          User.t() | :find_user_error
-  defp get_or_create_user(%{id: id, mbta_uuid: mbta_uuid} = id_map, email, phone_number, role) do
+  @spec get_or_create_user(%{id: User.id(), mbta_uuid: User.id() | nil}) ::
+          [User.t()]
+  defp get_or_create_user(id_map) do
     # This checks both the normal id from Keycloak, and the legacy mbta_uuid. We should get either 0 or 1 users back.
-    user_list = User.get_by_alternate_id(id_map)
-
-    case length(user_list) do
-      0 ->
-        # If neither ID is found, the user just created their account in Keycloak so we need to add them to our database
-        Repo.insert!(%User{
-          id: id,
-          email: email,
-          phone_number: phone_number,
-          role: role
-        })
-
-        User.get(id)
-
-      1 ->
-        # If 1 user is found, we want to return that user
-        hd(user_list)
-
-      2 ->
-        # If 2 users are found, something weird happened. Log and return nil. User will be redirected to landing page.
-        Logger.warn("User with 2 ids found. sub id: #{id}, mbta_uuid: #{mbta_uuid}")
-        :find_user_error
-    end
+    User.get_by_alternate_id(id_map)
   end
 
   # Display the email and phone number we received in the token rather than the
@@ -114,13 +106,12 @@ defmodule ConciergeSite.AuthController do
   # changed one of these fields in Keycloak, we might not have had time receive
   # that message yet, so the values in the token are more authoritative.
   @spec use_props_from_token(
-          User.t() | :find_user_error,
+          User.t(),
           String.t(),
           String.t() | nil,
           String.t()
         ) ::
-          User.t() | nil
-  defp use_props_from_token(:find_user_error, _, _, _), do: nil
+          User.t()
 
   defp use_props_from_token(user, email, phone_number, role) do
     %User{