refactor and add tests for user account management

lib/algora/accounts/accounts.ex

@@ -18,6 +18,7 @@ defmodule Algora.Accounts do
   alias Algora.Workspace.Ticket
 
   require Algora.SQL
+  require Logger
 
   def base_query, do: User
 
@@ -258,76 +259,62 @@ defmodule Algora.Accounts do
   def register_github_user(current_user, primary_email, info, emails, token) do
     query =
       from(u in User,
-        left_join: i in Identity,
-        on: i.provider == "github" and i.provider_id == ^to_string(info["id"]),
-        where:
-          (u.provider == "github" and u.provider_id == ^to_string(info["id"])) or
-            u.email == ^primary_email,
-        select: {u, i}
+        where: u.email == ^primary_email or (u.provider == "github" and u.provider_id == ^to_string(info["id"]))
       )
 
-    account =
-      case Repo.all(query) do
-        [] -> nil
-        [{user, identity}] -> {user, identity}
-        records -> Enum.find(records, fn {user, _} -> user.id == current_user.id end)
+    primary_user =
+      case {current_user, Repo.all(query)} do
+        {_, []} -> nil
+        {_, [user]} -> user
+        {nil, users} -> Enum.find(users, &(&1.email == primary_email))
+        {user, users} -> Enum.find(users, &(&1.id == user.id))
       end
 
-    case account do
+    case primary_user do
       nil -> create_user(info, primary_email, emails, token)
-      {user, identity} -> update_user(user, identity, info, primary_email, emails, token)
+      user -> update_user(user, info, primary_email, emails, token)
     end
   end
 
-  def register_org(params) do
-    params |> User.org_registration_changeset() |> Repo.insert()
-  end
-
   def create_user(info, primary_email, emails, token) do
     nil
     |> User.github_registration_changeset(info, primary_email, emails, token)
     |> Repo.insert()
   end
 
-  def update_user(user, identity, info, primary_email, emails, token) do
+  def update_user(user, info, primary_email, emails, token) do
     old_user = Repo.get_by(User, provider: "github", provider_id: to_string(info["id"]))
 
-    identity_changeset = Identity.github_registration_changeset(user, info, primary_email, emails, token)
-
-    user_changeset = User.github_registration_changeset(user, info, primary_email, emails, token)
-
     Repo.transact(fn ->
-      delete_result =
-        if identity do
-          Repo.delete(identity)
-        else
-          {:ok, nil}
-        end
+      Repo.delete_all(from(i in Identity, where: i.provider == "github" and i.provider_id == ^to_string(info["id"])))
 
-      migrate_result =
-        if old_user && old_user.id != user.id do
-          {:ok, old_user} =
-            old_user
-            |> change(provider: nil, provider_id: nil, provider_login: nil, provider_meta: nil)
-            |> Repo.update()
+      with true <- old_user && old_user.id != user.id,
+           {:ok, old_user} <- old_user |> change(provider: nil, provider_id: nil, provider_login: nil) |> Repo.update() do
+        migrate_user(old_user, user)
+      else
+        {:error, reason} ->
+          Logger.error("Failed to migrate user: #{inspect(reason)}")
 
-          # TODO: enqueue job
-          migrate_user(old_user, user)
+        _ ->
+          :ok
+      end
 
-          {:ok, old_user}
-        else
-          {:ok, nil}
-        end
+      identity_changeset = Identity.github_registration_changeset(user, info, primary_email, emails, token)
+      user_changeset = User.github_registration_changeset(user, info, primary_email, emails, token)
 
-      with {:ok, _} <- delete_result,
-           {:ok, _} <- migrate_result,
-           {:ok, _} <- Repo.insert(identity_changeset) do
-        Repo.update(user_changeset)
+      with {:ok, _} <- Repo.insert(identity_changeset),
+           {:ok, user} <- Repo.update(user_changeset) do
+        {:ok, user}
+      else
+        {:error, reason} ->
+          Logger.error("Failed to update user: #{inspect(reason)}")
+          {:error, reason}
       end
     end)
   end
 
   def migrate_user(old_user, new_user) do
+    # TODO: enqueue job
     Repo.update_all(
       from(r in Repository, where: r.user_id == ^old_user.id),
       set: [user_id: new_user.id]
@@ -357,6 +344,12 @@ defmodule Algora.Accounts do
       from(i in Installation, where: i.connected_user_id == ^old_user.id),
       set: [connected_user_id: new_user.id]
     )
+
+    :ok
+  end
+
+  def register_org(params) do
+    params |> User.org_registration_changeset() |> Repo.insert()
   end
 
   # def get_user_by_provider_email(provider, email) when provider in [:github] do

test/algora/accounts_test.exs

@@ -2,27 +2,146 @@ defmodule Algora.AccountsTest do
   use Algora.DataCase
 
   alias Algora.Accounts
+  alias Algora.Accounts.Identity
+  alias Algora.Accounts.User
   alias Algora.Organizations
+  alias Algora.Repo
+  alias Algora.Workspace.Repository
+
+  describe "register_github_user/5" do
+    setup do
+      # Sample GitHub info that would come from OAuth
+      github_info = %{
+        "id" => "12345",
+        "login" => "testuser",
+        "name" => "Test User",
+        "avatar_url" => "https://example.com/avatar.jpg",
+        "bio" => "Test bio"
+      }
 
-  describe "accounts" do
-    test "register github user" do
-      email = "[email protected]"
+      emails = [
+        %{"email" => "[email protected]", "primary" => true, "verified" => true},
+        %{"email" => "[email protected]", "primary" => false, "verified" => true}
+      ]
+
+      token = "github_oauth_token_123"
+      primary_email = "[email protected]"
+
+      {:ok,
+       %{
+         github_info: github_info,
+         emails: emails,
+         token: token,
+         primary_email: primary_email
+       }}
+    end
 
-      info = %{
-        "id" => "1234",
-        "login" => "githubuser",
-        "name" => "Github User"
-      }
+    test "creates new user when no matching accounts exist", %{
+      github_info: github_info,
+      emails: emails,
+      token: token,
+      primary_email: primary_email
+    } do
+      {:ok, user} = Accounts.register_github_user(nil, primary_email, github_info, emails, token)
+
+      assert user.provider == "github"
+      assert user.provider_id == "12345"
+      assert user.email == primary_email
+      assert user.provider_login == "testuser"
+
+      identity = Repo.get_by(Identity, user_id: user.id)
+      assert identity.provider == "github"
+      assert identity.provider_token == token
+    end
+
+    test "updates existing user when matching by GitHub ID", %{
+      github_info: github_info,
+      emails: emails,
+      token: token,
+      primary_email: primary_email
+    } do
+      {:ok, existing_user} = Accounts.register_github_user(nil, "[email protected]", github_info, emails, "old_token")
+
+      identity = Repo.get_by(Identity, user_id: existing_user.id)
+      assert identity.provider_token == "old_token"
+
+      {:ok, updated_user} = Accounts.register_github_user(existing_user, primary_email, github_info, emails, token)
+
+      assert updated_user.id == existing_user.id
+      assert updated_user.email == "[email protected]"
+      assert updated_user.provider_login == "testuser"
+
+      assert Repo.get(Identity, identity.id) == nil
+      assert Repo.get_by(Identity, user_id: updated_user.id).provider_token == token
+    end
+
+    test "links accounts when current user email matches GitHub email", %{
+      github_info: github_info,
+      emails: emails,
+      token: token,
+      primary_email: primary_email
+    } do
+      # Create existing user with matching email but no GitHub connection
+      existing_user = insert!(:user, email: primary_email, display_name: "Existing User")
+
+      {:ok, updated_user} = Accounts.register_github_user(existing_user, primary_email, github_info, emails, token)
 
-      {:ok, user} = Accounts.register_github_user(nil, email, info, [email], "token123")
-      {:ok, user_again} = Accounts.register_github_user(user, email, info, [email], "token123")
+      assert updated_user.id == existing_user.id
+      assert updated_user.provider == "github"
+      assert updated_user.provider_id == "12345"
+
+      identity = Repo.get_by(Identity, user_id: updated_user.id)
+      assert identity.provider == "github"
+      assert identity.provider_token == token
+    end
+
+    test "migrates data when merging duplicate accounts", %{
+      github_info: github_info,
+      emails: emails,
+      token: token,
+      primary_email: primary_email
+    } do
+      # Create a GitHub-connected user
+      {:ok, github_user} = Accounts.register_github_user(nil, "[email protected]", github_info, emails, "old_token")
+
+      # Create another user with matching email
+      email_user = insert!(:user, email: primary_email, display_name: "Email User")
+
+      # Create some associated data for the GitHub user
+      repository = insert!(:repository, user: github_user, name: "test-repo")
+
+      {:ok, merged_user} = Accounts.register_github_user(email_user, primary_email, github_info, emails, token)
+
+      # Verify the accounts were merged
+      assert merged_user.id == email_user.id
+
+      # Verify associated data was migrated
+      updated_repository = Repo.get(Repository, repository.id)
+      assert updated_repository.user_id == merged_user.id
+
+      # Verify the old user's GitHub connection was removed
+      old_user = Repo.get(User, github_user.id)
+      assert is_nil(old_user.provider)
+      assert is_nil(old_user.provider_id)
+    end
+
+    test "is idempotent and creates activities", %{
+      github_info: github_info,
+      emails: emails,
+      token: token,
+      primary_email: primary_email
+    } do
+      {:ok, user} = Accounts.register_github_user(nil, primary_email, github_info, emails, token)
+      {:ok, user_again} = Accounts.register_github_user(user, primary_email, github_info, emails, token)
 
       assert user.id == user_again.id
       assert_activity_names([:identity_created, :identity_created])
       assert_activity_names_for_user(user.id, [:identity_created])
       assert_activity_names_for_user(user_again.id, [:identity_created])
     end
+  end
 
+  describe "accounts" do
     test "query" do
       user_1 = insert(:user)
       user_2 = insert(:user, tech_stack: ["rust", "c++"])