Merge pull request #997 from code-corps/980-fix-email-overwriting-by-github

Fix email overwriting by GitHub

Author: joshsmith

lib/code_corps/accounts/accounts.ex

@@ -2,35 +2,103 @@ defmodule CodeCorps.Accounts do
   @moduledoc ~S"""
   Main entry-point for managing accounts.
 
-  All actions to acounts should go through here.
+  All actions to accounts should go through here.
   """
 
   alias CodeCorps.{
+    Accounts.Changesets,
+    Comment,
     GitHub.Adapters,
+    GithubAppInstallation,
     User,
-    Repo
+    Repo,
+    Task
   }
-  alias Ecto.Changeset
+  alias Ecto.{Changeset, Multi}
+
+  import Ecto.Query
 
   @doc ~S"""
   Creates a user record using attributes from a GitHub payload.
   """
   @spec create_from_github(map) :: {:ok, User.t} | {:error, Changeset.t}
   def create_from_github(%{} = attrs) do
     %User{}
-    |> create_from_github_changeset(attrs)
+    |> Changesets.create_from_github_changeset(attrs)
     |> Repo.insert
   end
 
   @doc ~S"""
-  Casts a changeset used for creating a user account from a github user payload
+  Updates a user record using attributes from a GitHub payload along with the
+  access token.
   """
-  @spec create_from_github_changeset(struct, map) :: Changeset.t
-  def create_from_github_changeset(struct, %{} = params) do
-    struct
-    |> Changeset.change(params |> Adapters.User.from_github_user())
-    |> Changeset.put_change(:context, "github")
-    |> Changeset.unique_constraint(:email)
-    |> Changeset.validate_inclusion(:type, ["bot", "user"])
+  @spec update_from_github_oauth(User.t, map, String.t) :: {:ok, User.t} | {:error, Changeset.t}
+  def update_from_github_oauth(%User{} = user, %{} = params, access_token) do
+    params =
+      params
+      |> Adapters.User.from_github_user()
+      |> Map.put(:github_auth_token, access_token)
+
+    changeset = user |> Changesets.update_from_github_oauth_changeset(params)
+
+    multi =
+      Multi.new
+      |> Multi.update(:user, changeset)
+      |> Multi.run(:installations, fn %{user: %User{} = user} -> user |> associate_installations() end)
+      |> Multi.run(:tasks, fn %{user: %User{} = user} -> user |> associate_tasks() end)
+      |> Multi.run(:comments, fn %{user: %User{} = user} -> user |> associate_comments() end)
+
+    case Repo.transaction(multi) do
+      {:ok, %{user: %User{} = user, installations: installations}} ->
+        {:ok, user |> Map.put(:github_app_installations, installations)}
+      {:error, :user, %Changeset{} = changeset, _actions_done} ->
+        {:error, changeset}
+    end
+  end
+
+  @spec associate_installations(User.t) :: {:ok, list(GithubAppInstallation.t)}
+  defp associate_installations(%User{id: user_id, github_id: github_id}) do
+    updates = [set: [user_id: user_id]]
+    update_options = [returning: true]
+
+    GithubAppInstallation
+    |> where([i], i.sender_github_id == ^github_id)
+    |> where([i], is_nil(i.user_id))
+    |> Repo.update_all(updates, update_options)
+    |> (fn {_count, installations} -> {:ok, installations} end).()
+  end
+
+  @spec associate_tasks(User.t) :: {:ok, list(Task.t)}
+  defp associate_tasks(%User{id: user_id, github_id: github_id}) do
+    updates = [set: [user_id: user_id]]
+    update_options = [returning: true]
+
+    existing_user_ids =
+      User
+      |> where(github_id: ^github_id)
+      |> select([u], u.id)
+      |> Repo.all
+
+    Task
+    |> where([t], t.user_id in ^existing_user_ids)
+    |> Repo.update_all(updates, update_options)
+    |> (fn {_count, tasks} -> {:ok, tasks} end).()
+  end
+
+  @spec associate_comments(User.t) :: {:ok, list(Comment.t)}
+  defp associate_comments(%User{id: user_id, github_id: github_id}) do
+    updates = [set: [user_id: user_id]]
+    update_options = [returning: true]
+
+    existing_user_ids =
+      User
+      |> where(github_id: ^github_id)
+      |> select([u], u.id)
+      |> Repo.all
+
+    Comment
+    |> where([c], c.user_id in ^existing_user_ids)
+    |> Repo.update_all(updates, update_options)
+    |> (fn {_count, comments} -> {:ok, comments} end).()
   end
 end

lib/code_corps/accounts/changesets.ex

@@ -0,0 +1,41 @@
+defmodule CodeCorps.Accounts.Changesets do
+  @moduledoc ~S"""
+  Changesets for Code Corps accounts.
+  """
+
+  alias CodeCorps.GitHub.Adapters
+  alias Ecto.Changeset
+
+  @doc ~S"""
+  Casts a changeset used for creating a user account from a github user payload
+  """
+  @spec create_from_github_changeset(struct, map) :: Changeset.t
+  def create_from_github_changeset(struct, %{} = params) do
+    struct
+    |> Changeset.change(params |> Adapters.User.from_github_user())
+    |> Changeset.put_change(:sign_up_context, "github")
+    |> Changeset.unique_constraint(:email)
+    |> Changeset.validate_inclusion(:type, ["bot", "user"])
+  end
+
+  @doc ~S"""
+  Casts a changeset used for creating a user account from a github user payload
+  """
+  @spec update_from_github_oauth_changeset(struct, map) :: Changeset.t
+  def update_from_github_oauth_changeset(struct, %{} = params) do
+    struct
+    |> Changeset.cast(params, [:github_auth_token, :github_avatar_url, :github_id, :github_username, :type])
+    |> ensure_email_without_overwriting(params)
+    |> Changeset.unique_constraint(:email)
+    |> Changeset.validate_required([:github_auth_token, :github_avatar_url, :github_id, :github_username, :type])
+  end
+
+  @spec ensure_email_without_overwriting(Changeset.t, map) :: Changeset.t
+  defp ensure_email_without_overwriting(%Changeset{} = changeset, %{"email" => new_email} = _params) do
+    case changeset |> Changeset.get_field(:email) do
+      nil -> changeset |> Changeset.put_change(:email, new_email)
+      _email -> changeset
+    end
+  end
+  defp ensure_email_without_overwriting(%Changeset{} = changeset, _params), do: changeset
+end

lib/code_corps/github/user.ex

@@ -3,11 +3,9 @@ defmodule CodeCorps.GitHub.User do
   Used to perform user actions on the github API
   """
 
-  alias CodeCorps.{Comment, GitHub, GithubAppInstallation, Repo, Task, User}
-  alias CodeCorps.GitHub.Adapters.User, as: UserAdapter
-  alias Ecto.{Changeset, Multi}
+  alias CodeCorps.{Accounts, GitHub, User}
+  alias Ecto.{Changeset}
 
-  import Ecto.Query
 
   @single_endpoint "user"
 
@@ -42,73 +40,7 @@ defmodule CodeCorps.GitHub.User do
   @spec do_connect(User.t, map, String.t) :: {:ok, User.t} | {:error, Changeset.t}
   defp do_connect(%User{} = user, %{} = user_payload, access_token)
     when is_binary(access_token) do
-
-    changeset =
-      user
-      |> Changeset.change(user_payload |> UserAdapter.from_github_user)
-      |> Changeset.put_change(:github_auth_token, access_token)
-      |> Changeset.validate_required([:github_auth_token, :github_avatar_url, :github_id, :github_username])
-
-    multi =
-      Multi.new
-      |> Multi.update(:user, changeset)
-      |> Multi.run(:installations, fn %{user: %User{} = user} -> user |> associate_installations() end)
-      |> Multi.run(:tasks, fn %{user: %User{} = user} -> user |> associate_tasks() end)
-      |> Multi.run(:comments, fn %{user: %User{} = user} -> user |> associate_comments() end)
-
-    case Repo.transaction(multi) do
-      {:ok, %{user: %User{} = user, installations: installations}} ->
-        {:ok, user |> Map.put(:github_app_installations, installations)}
-      {:error, :user, %Changeset{} = changeset, _actions_done} ->
-        {:error, changeset}
-    end
-  end
-
-
-  @spec associate_installations(User.t) :: {:ok, list(GithubAppInstallation.t)}
-  defp associate_installations(%User{id: user_id, github_id: github_id}) do
-    updates = [set: [user_id: user_id]]
-    update_options = [returning: true]
-
-    GithubAppInstallation
-    |> where([i], i.sender_github_id == ^github_id)
-    |> where([i], is_nil(i.user_id))
-    |> Repo.update_all(updates, update_options)
-    |> (fn {_count, installations} -> {:ok, installations} end).()
-  end
-
-  @spec associate_tasks(User.t) :: {:ok, list(Task.t)}
-  defp associate_tasks(%User{id: user_id, github_id: github_id}) do
-    updates = [set: [user_id: user_id]]
-    update_options = [returning: true]
-
-    existing_user_ids =
-      User
-      |> where(github_id: ^github_id)
-      |> select([u], u.id)
-      |> Repo.all
-
-    Task
-    |> where([t], t.user_id in ^existing_user_ids)
-    |> Repo.update_all(updates, update_options)
-    |> (fn {_count, tasks} -> {:ok, tasks} end).()
-  end
-
-  @spec associate_comments(User.t) :: {:ok, list(Comment.t)}
-  defp associate_comments(%User{id: user_id, github_id: github_id}) do
-    updates = [set: [user_id: user_id]]
-    update_options = [returning: true]
-
-    existing_user_ids =
-      User
-      |> where(github_id: ^github_id)
-      |> select([u], u.id)
-      |> Repo.all
-
-    Comment
-    |> where([c], c.user_id in ^existing_user_ids)
-    |> Repo.update_all(updates, update_options)
-    |> (fn {_count, comments} -> {:ok, comments} end).()
+    Accounts.update_from_github_oauth(user, user_payload, access_token)
   end
 
   @doc ~S"""

priv/repo/seeds.exs

@@ -1,7 +1,6 @@
 alias CodeCorps.{
   Category, Organization, ProjectUser, Project, ProjectCategory, ProjectSkill,
-  Repo, Role, Skill, SluggedRoute, Task, User, UserCategory, UserRole,
-  UserSkill
+  Repo, Role, Skill, Task, User, UserCategory, UserRole, UserSkill
 }
 
 users = [

test/lib/code_corps/accounts/accounts_test.exs

@@ -14,26 +14,38 @@ defmodule CodeCorps.AccountsTest do
         |> Accounts.create_from_github
 
       assert user.id
-      assert user.context == "github"
+      assert user.sign_up_context == "github"
       assert user.type == "user"
     end
 
-    test "returns changeset if there was a validation error" do
+    test "validates the uniqueness of email" do
       %{"email" => email} = payload = TestHelpers.load_endpoint_fixture("user")
-      # email must be unique, so if a user with email already exists, this
-      # triggers a validation error
+
+      # Ensure a user exists so there's a duplicate email
       insert(:user, email: email)
 
-      {:error, %Changeset{} = changeset} = payload |> Accounts.create_from_github
+      {:error, %Changeset{} = changeset} =
+        payload
+        |> Accounts.create_from_github
+        
       assert changeset.errors[:email] == {"has already been taken", []}
     end
   end
 
-  describe "create_from_github_changeset/1" do
-    test "validates inclusion of type" do
-      params = %{"email" => "test@email.com", "type" => "Organization"}
-      changeset = Accounts.create_from_github_changeset(%User{}, params)
-      assert changeset.errors[:type] == {"is invalid", [validation: :inclusion]}
+  describe "update_from_github_oauth/3" do
+    test "updates proper user from provided payload" do
+      user = insert(:user)
+      params = TestHelpers.load_endpoint_fixture("user")
+      token = "random_token"
+
+      {:ok, %User{} = user} =
+        user
+        |> Accounts.update_from_github_oauth(params, token)
+
+      assert user.id
+      assert user.github_auth_token == token
+      assert user.sign_up_context == "default"
+      assert user.type == "user"
     end
   end
 end

test/lib/code_corps/accounts/changesets_test.exs

@@ -0,0 +1,65 @@
+defmodule CodeCorps.Accounts.ChangesetsTest do
+  @moduledoc false
+
+  use CodeCorps.DbAccessCase
+
+  alias CodeCorps.{Accounts, User}
+
+  describe "create_from_github_changeset/1" do
+    test "validates inclusion of type" do
+      params = %{"email" => "test@email.com", "type" => "Organization"}
+
+      changeset =
+        %User{}
+        |> Accounts.Changesets.create_from_github_changeset(params)
+
+      assert changeset.errors[:type] == {"is invalid", [validation: :inclusion]}
+    end
+  end
+
+  describe "update_from_github_oauth_changeset/2" do
+    test "ensures an email is not overridden when the user has an email" do
+      user = insert(:user, email: "original@email.com")
+      params = %{"email" => "new@email.com"}
+
+      changeset =
+        user
+        |> Accounts.Changesets.update_from_github_oauth_changeset(params)
+
+      refute changeset.changes[:email]
+    end
+
+    test "ensures an email is not set to nil" do
+      user = insert(:user, email: "original@email.com")
+      params = %{"email" => nil}
+
+      changeset =
+        user
+        |> Accounts.Changesets.update_from_github_oauth_changeset(params)
+
+      refute changeset.changes[:email]
+    end
+
+    test "ensures an email is set when initially nil" do
+      user = insert(:user, email: nil)
+      params = %{"email" => "new@email.com"}
+
+      changeset =
+        user
+        |> Accounts.Changesets.update_from_github_oauth_changeset(params)
+
+      assert changeset.changes[:email]
+    end
+
+    test "works without email params" do
+      user = insert(:user)
+      params = %{}
+
+      changeset =
+        user
+        |> Accounts.Changesets.update_from_github_oauth_changeset(params)
+
+      refute changeset.errors[:email]
+    end
+  end
+end