Merge pull request #967 from code-corps/939-correct-user-assignment-w…

…hen-syncing-task

Fix user linking of github issues and comments to account for bot user

lib/code_corps/accounts/accounts.ex

@@ -18,9 +18,19 @@ defmodule CodeCorps.Accounts do
   @spec create_from_github(map) :: {:ok, User.t} | {:error, Changeset.t}
   def create_from_github(%{} = attrs) do
     %User{}
-    |> Changeset.change(attrs |> Adapters.User.from_github_user())
+    |> create_from_github_changeset(attrs)
+    |> Repo.insert
+  end
+
+  @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(:context, "github")
     |> Changeset.unique_constraint(:email)
-    |> Repo.insert
+    |> Changeset.validate_inclusion(:type, ["bot", "user"])
   end
 end

lib/code_corps/github/adapters/user.ex

@@ -8,21 +8,30 @@ defmodule CodeCorps.GitHub.Adapters.User do
     {:github_avatar_url, ["avatar_url"]},
     {:github_id, ["id"]},
     {:github_username, ["login"]},
-    {:email, ["email"]}
+    {:email, ["email"]},
+    {:type, ["type"]}
   ]
 
   @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.
+  Any `nil` values are removed here. For example, we don't want to delete
+  an existing email just because the GitHub payload is missing that data.
+
+  The `type` gets transformed to match our expected values for user type.
   """
   @spec from_github_user(map) :: map
   def from_github_user(%{} = payload) do
     payload
     |> CodeCorps.Adapter.MapTransformer.transform(@mapping)
     |> Enum.reject(fn {_, v} -> is_nil(v) end)
     |> Map.new
+    |> transform_type
   end
+
+  @spec transform_type(map) :: map
+  defp transform_type(%{:type => "Bot"} = map), do: Map.put(map, :type, "bot")
+  defp transform_type(%{:type => "User"} = map), do: Map.put(map, :type, "user")
+  defp transform_type(map), do: map
 end

lib/code_corps/github/event/issue_comment/user_linker.ex

@@ -1,29 +1,66 @@
 defmodule CodeCorps.GitHub.Event.IssueComment.UserLinker do
   @moduledoc ~S"""
-  In charge of finding a user to link with a Task when processing an
+  In charge of finding a user to link with a Comment when processing an
   IssueComment webhook.
   """
 
+  import Ecto.Query
+
   alias CodeCorps.{
     Accounts,
+    Comment,
     Repo,
     User
   }
 
+  @typep linking_result :: {:ok, User.t} |
+                           {:error, Ecto.Changeset.t}
+                           {:error, :multiple_users}
+
   @doc ~S"""
-  Finds or creates a user using information contained in an Issues webhook
-  payload
+  Finds or creates a user using information contained in a GitHub IssueComment
+  webhook payload.
+
+  The process is as follows:
+  - Find all affected comments and extract their user data.
+  - Search for the user in our database.
+    - If we match a single user, then the comment should be for that user.
+    - If there are no matching users, then the comment was created on Github by
+      someone who does not have a matching GitHub-connected Code Corps account.
+      We create a placeholder user account until that GitHub user is claimed by
+      a Code Corps user.
+  created.
+    - If there are multiple matching users, this is an unexpected scenario and
+      should error out.
   """
   @spec find_or_create_user(map) :: {:ok, User.t}
-  def find_or_create_user(%{"comment" => %{"user" => user_attrs}}) do
-    case user_attrs |> find_user() do
-      nil -> user_attrs |> Accounts.create_from_github
-      %User{} = user -> {:ok, user}
-    end
+  def find_or_create_user(%{"comment" => %{"user" => user_attrs}} = attrs) do
+    attrs
+    |> match_users
+    |> marshall_response(user_attrs)
+  end
+
+  @spec match_users(map) :: list(User.t)
+  defp match_users(%{"comment" => %{"id" => github_id}}) do
+    query = from u in User,
+      distinct: u.id,
+      join: c in Comment, on: u.id == c.user_id, where: c.github_id == ^github_id
+
+    query |> Repo.all
+  end
+
+  @spec marshall_response(list, map) :: linking_result
+  defp marshall_response([%User{} = single_user], %{}), do: {:ok, single_user}
+  defp marshall_response([], %{} = user_attrs) do
+    user_attrs |> find_or_create_disassociated_user()
   end
+  defp marshall_response([_head | _tail], %{}), do: {:error, :multiple_users}
 
-  @spec find_user(map) :: User.t | nil
-  defp find_user(%{"id" => github_id}) do
-    User |> Repo.get_by(github_id: github_id)
+  @spec find_or_create_disassociated_user(map) :: {:ok, User.t}
+  def find_or_create_disassociated_user(%{"id" => github_id} = attrs) do
+    case User |> Repo.get_by(github_id: github_id) do
+      nil -> attrs |> Accounts.create_from_github
+      %User{} = user -> {:ok, user}
+    end
   end
 end

lib/code_corps/github/event/issues/user_linker.ex

@@ -2,28 +2,73 @@ defmodule CodeCorps.GitHub.Event.Issues.UserLinker do
   @moduledoc ~S"""
   In charge of finding a user to link with a Task when processing an Issues
   webhook.
+
+  The only entry point is `find_or_create_user/1`.
   """
 
+  import Ecto.Query
+
   alias CodeCorps.{
     Accounts,
+    GithubRepo,
     Repo,
+    Task,
     User
   }
 
+  @typep linking_result :: {:ok, User.t} |
+                           {:error, Ecto.Changeset.t}
+                           {:error, :multiple_users}
+
   @doc ~S"""
-  Finds or creates a user using information contained in an Issues webhook
-  payload
+  Finds or creates a user using information contained in a GitHub Issue
+  webhook payload.
+
+  The process is as follows:
+  - Find all affected tasks and extract their user data.
+  - Search for the user in our database.
+    - If we match a single user, then the issue should be related to that user.
+    - If there are no matching users, then the issue was created on Github by
+      someone who does not have a matching GitHub-connected Code Corps account.
+      We create a placeholder user account until that GitHub user is claimed by
+      a Code Corps user.
+  created.
+    - If there are multiple matching users, this is an unexpected scenario and
+      should error out.
   """
-  @spec find_or_create_user(map) :: {:ok, User.t}
-  def find_or_create_user(%{"issue" => %{"user" => user_attrs}}) do
-    case user_attrs |> find_user() do
-      nil -> user_attrs |> Accounts.create_from_github
-      %User{} = user -> {:ok, user}
-    end
+  @spec find_or_create_user(map) :: linking_result
+  def find_or_create_user(%{"issue" => %{"user" => user_attrs}} = attrs) do
+    attrs
+    |> match_users
+    |> marshall_response(user_attrs)
+  end
+
+  @spec match_users(map) :: list(User.t)
+  defp match_users(
+    %{
+      "issue" => %{"number" => github_issue_number},
+      "repository" =>%{"id" => github_repo_id}}) do
+
+    query = from u in User,
+      distinct: u.id,
+      join: t in Task, on: u.id == t.user_id, where: t.github_issue_number == ^github_issue_number,
+      join: r in GithubRepo, on: r.id == t.github_repo_id, where: r.github_id == ^github_repo_id
+
+    query |> Repo.all
   end
 
-  @spec find_user(map) :: User.t | nil
-  defp find_user(%{"id" => github_id}) do
-    User |> Repo.get_by(github_id: github_id)
+  @spec marshall_response(list, map) :: linking_result
+  defp marshall_response([%User{} = single_user], %{}), do: {:ok, single_user}
+  defp marshall_response([], %{} = user_attrs) do
+    user_attrs |> find_or_create_disassociated_user()
+  end
+  defp marshall_response([_head | _tail], %{}), do: {:error, :multiple_users}
+
+  @spec find_or_create_disassociated_user(map) :: {:ok, User.t}
+  def find_or_create_disassociated_user(%{"id" => github_id} = attrs) do
+    case User |> Repo.get_by(github_id: github_id) do
+      nil -> attrs |> Accounts.create_from_github
+      %User{} = user -> {:ok, user}
+    end
   end
 end

lib/code_corps/model/user.ex

@@ -33,6 +33,7 @@ defmodule CodeCorps.User do
     field :password_confirmation, :string, virtual: true
     field :sign_up_context, :string, default: "default"
     field :twitter, :string
+    field :type, :string, default: "user"
     field :username, :string
     field :website, :string
     field :state, :string, default: "signed_up"
@@ -88,6 +89,7 @@ defmodule CodeCorps.User do
     |> validate_slug(:username)
     |> unique_constraint(:username, name: :users_lower_username_index)
     |> unique_constraint(:email)
+    |> put_change(:type, "user")
     |> put_pass_hash()
     |> put_slugged_route()
     |> generate_icon_color(:default_color)

priv/repo/migrations/20170925214512_add_type_to_users.exs

@@ -0,0 +1,9 @@
+defmodule CodeCorps.Repo.Migrations.AddTypeToUsers do
+  use Ecto.Migration
+
+  def change do
+    alter table(:users) do
+      add :type, :string, default: "user"
+    end
+  end
+end