PR feedback

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]
     )

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]

apps/concierge_site/lib/controllers/auth_controller.ex

@@ -36,7 +36,7 @@ defmodule ConciergeSite.AuthController do
     user =
       %{id: id, mbta_uuid: mbta_uuid}
       |> get_or_create_user(email, phone_number, role)
-      |> use_props_from_token(email, phone_number, role)
+
 
     logout_params = %{
       post_logout_redirect_uri: page_url(conn, :landing)
@@ -49,6 +49,7 @@ defmodule ConciergeSite.AuthController do
         SessionHelper.sign_out(conn, skip_oidc_sign_out: true)
 
       _ ->
+        user = use_props_from_token(user, email, phone_number, role)
         conn
         |> put_session("logout_uri", logout_uri)
         |> SessionHelper.sign_in(user)
@@ -74,12 +75,12 @@ defmodule ConciergeSite.AuthController do
   end
 
   @spec get_or_create_user(
-          %{id: User.id() | nil, mbta_uuid: User.id() | nil},
+          %{id: User.id(), mbta_uuid: User.id() | nil},
           String.t(),
           String.t() | nil,
           String.t()
         ) ::
-          User.t() | :find_user_error
+          User.t() | nil
   defp get_or_create_user(%{id: id, mbta_uuid: mbta_uuid} = id_map, email, phone_number, role) 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)
@@ -103,7 +104,7 @@ defmodule ConciergeSite.AuthController do
       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
+        nil
     end
   end
 
@@ -114,14 +115,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
-
   defp use_props_from_token(user, email, phone_number, role) do
     %User{
       user