Merge pull request code-corps#859 from code-corps/858-associate-connected-user-with-installations

begedin · web-flow · commit 5324a28ffcea · 2017-07-31T10:32:51.000+02:00
Associate connected user with installations
diff --git a/lib/code_corps/github/adapters/user.ex b/lib/code_corps/github/adapters/user.ex
@@ -7,11 +7,22 @@ defmodule CodeCorps.GitHub.Adapters.User do
   @mapping [
     {:github_avatar_url, ["avatar_url"]},
     {:github_id, ["id"]},
-    {:github_username, ["login"]}
+    {:github_username, ["login"]},
+    {:email, ["email"]}
   ]
 
+  @doc ~S"""
+  Converts a Github user payload into a map of attributes suitable for creating
+  or updating a `CodeCorps.User`
+
+  Any nil values are removed here, since we do not want to, for example, delete
+  an existing email just because the github payload doesn't have that data.
+  """
   @spec from_github_user(map) :: map
   def from_github_user(%{} = payload) do
-    payload |> CodeCorps.Adapter.MapTransformer.transform(@mapping)
+    payload
+    |> CodeCorps.Adapter.MapTransformer.transform(@mapping)
+    |> Enum.reject(fn {_, v} -> is_nil(v) end)
+    |> Map.new
   end
 end
diff --git a/lib/code_corps/github/oauth.ex b/lib/code_corps/github/oauth.ex
diff --git a/lib/code_corps/github/user.ex b/lib/code_corps/github/user.ex
@@ -1,26 +1,85 @@
 defmodule CodeCorps.GitHub.User do
   @moduledoc """
   Used to perform user actions on the github API
-  Also defines a GitHub.User struct
   """
-  defstruct [:avatar_url, :email, :id, :login]
 
-  @type t :: %__MODULE__{}
+  alias CodeCorps.{GitHub, GithubAppInstallation, Repo, User}
+  alias CodeCorps.GitHub.Adapters.User, as: UserAdapter
+  alias Ecto.{Changeset, Multi}
 
-  alias CodeCorps.GitHub
+  import Ecto.Query
 
   @single_endpoint "user"
 
-  def new() do
+  @doc """
+  POSTs `code` and `state` to GitHub to receive an OAuth token,
+  then associates the given user with that OAuth token.
 
+  Also associates any orphaned `GithubAppInstallation` records matching their
+  `sender_github_id` field with the user's `github_id`
+
+  Returns one of the following:
+
+  - `{:ok, %CodeCorps.User{}}`
+  - `{:error, %Ecto.Changeset{}}`
+  - `{:error, GitHub.api_error_struct}`
+  """
+  @spec connect(User.t, String.t, String.t) ::
+    {:ok, User.t} | {:error, Changeset.t} | {:error, GitHub.api_error_struct}
+  def connect(%User{} = user, code, state) do
+    with {:ok, %{"access_token" => access_token}} <- GitHub.user_access_token_request(code, state),
+         {:ok, %{} = user_payload} <- access_token |> GitHub.User.me()
+    do
+       user |> do_connect(user_payload, access_token)
+    else
+      {:error, error} -> {:error, error}
+    end
   end
 
+  @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)
+
+    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
+
+  @doc ~S"""
+  Requests the currently authenticated user payload from github
+  """
+  @spec me(String.t, Keyword.t) :: {:ok, map} | {:error, GitHub.api_error_struct}
   def me(access_token, opts \\ []) do
     case GitHub.Request.retrieve(@single_endpoint, opts ++ [access_token: access_token]) do
-      {:ok, %{"error" => error}} ->
-        {:error, error}
-      {:ok, %{"avatar_url" => avatar_url, "email" => email, "id" => id, "login" => login}} ->
-        {:ok, %__MODULE__{avatar_url: avatar_url, email: email, id: id, login: login}}
+      {:ok, %{"error" => error}} -> {:error, error}
+      {:ok, %{} = user_payload} -> {:ok, user_payload}
     end
   end
 end
diff --git a/test/lib/code_corps/github/oauth_test.exs b/test/lib/code_corps/github/oauth_test.exs
diff --git a/test/lib/code_corps/github/user_test.exs b/test/lib/code_corps/github/user_test.exs
@@ -0,0 +1,56 @@
+defmodule CodeCorps.GitHub.UserTest do
+  @moduledoc false
+
+  use CodeCorps.ModelCase
+  use CodeCorps.GitHubCase
+
+  alias CodeCorps.{GitHub, GithubAppInstallation, User}
+
+  @user_data %{
+    "avatar_url" => "foo_url",
+    "email" => "foo_email",
+    "id" => 123,
+    "login" => "foo_login"
+  }
+
+  @token_data %{"access_token" => "foo_auth_token"}
+
+  describe "connect/2" do
+    @tag bypass: %{"/user" => {200, @user_data}, "/" => {200, @token_data}}
+    test "posts to github, associates user and installations, returns updated user" do
+      user = insert(:user)
+
+      # 3 test installations
+      # this one should associate, because it's orphaned and matches github id
+      installation_1 = insert(:github_app_installation, user: nil, sender_github_id: 123)
+      # this one matches github id, but is not orphaned, so should not associate
+      installation_2 = insert(:github_app_installation, sender_github_id: 123)
+      # this one is orphaned, but does not match github id, should not associate
+      installation_3 = insert(:github_app_installation, user: nil, sender_github_id: 234)
+
+      {:ok, %User{} = returned_user} = GitHub.User.connect(user, "foo_code", "foo_state")
+
+      assert returned_user.id == user.id
+      assert returned_user.github_auth_token == "foo_auth_token"
+      assert returned_user.github_avatar_url == "foo_url"
+      assert returned_user.email == "foo_email"
+      assert returned_user.github_id == 123
+      assert returned_user.github_username == "foo_login"
+
+      assert Enum.count(returned_user.github_app_installations) == 1
+
+      assert Repo.get(GithubAppInstallation, installation_1.id).user_id == returned_user.id
+      refute Repo.get(GithubAppInstallation, installation_2.id).user_id == returned_user.id
+      refute Repo.get(GithubAppInstallation, installation_3.id).user_id == returned_user.id
+    end
+
+    @error_data %{"error" => "Not Found"}
+
+    @tag bypass: %{"/" => {404, @error_data}}
+    test "posts to github, returns error if reply is not ok" do
+      user = insert(:user)
+      error = GitHub.APIError.new({404, %{"message" => "{\"error\":\"Not Found\"}"}})
+      assert {:error, error} == GitHub.User.connect(user, "foo_code", "foo_state")
+    end
+  end
+end
diff --git a/test/models/user_test.exs b/test/models/user_test.exs
@@ -140,20 +140,6 @@ defmodule CodeCorps.UserTest do
     end
   end
 
-  describe "github_association_changeset" do
-    test "should cast github_auth_token" do
-      user = insert(:user)
-      changeset = user |> User.github_association_changeset(%{github_auth_token: "foo_token", github_avatar_url: "foo_url", github_id: 123, github_username: "foo_username"})
-      assert changeset.valid?
-      assert changeset.changes.github_auth_token == "foo_token"
-    end
-    test "github_auth_token should be required" do
-      user = insert(:user)
-      changeset = user |> User.github_association_changeset(%{})
-      refute changeset.valid?
-    end
-  end
-
   test "reset_password_changeset with valid passwords" do
     changeset = User.reset_password_changeset(%User{}, %{password: "foobar", password_confirmation: "foobar"})
     assert changeset.valid?
diff --git a/web/controllers/user_controller.ex b/web/controllers/user_controller.ex
@@ -40,7 +40,7 @@ defmodule CodeCorps.UserController do
   """
   def github_oauth(conn, %{"code" => code, "state" => state}) do
     current_user = Guardian.Plug.current_resource(conn)
-    with {:ok, user} <- GitHub.OAuth.connect(current_user, code, state)
+    with {:ok, user} <- GitHub.User.connect(current_user, code, state)
     do
       conn |> render("show.json-api", data: user)
     else
diff --git a/web/models/user.ex b/web/models/user.ex
@@ -103,12 +103,6 @@ defmodule CodeCorps.User do
     |> apply_state_transition(struct)
   end
 
-  def github_association_changeset(struct, params) do
-    struct
-    |> cast(params, [:github_auth_token, :github_avatar_url, :github_email, :github_id, :github_username])
-    |> validate_required([:github_auth_token, :github_avatar_url, :github_id, :github_username])
-  end
-
   def reset_password_changeset(struct, params) do
     struct
     |> cast(params, [:password, :password_confirmation])
