diff --git a/lib/algora/accounts/accounts.ex b/lib/algora/accounts/accounts.ex index 3e56d0bfb..affc09b48 100644 --- a/lib/algora/accounts/accounts.ex +++ b/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 diff --git a/test/algora/accounts_test.exs b/test/algora/accounts_test.exs index 0f42d51bc..b71cad5a9 100644 --- a/test/algora/accounts_test.exs +++ b/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 = "githubuser@example.com" + emails = [ + %{"email" => "test@example.com", "primary" => true, "verified" => true}, + %{"email" => "other@example.com", "primary" => false, "verified" => true} + ] + + token = "github_oauth_token_123" + primary_email = "test@example.com" + + {: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, "old@example.com", 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 == "old@example.com" + 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, "other@example.com", 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++"])