Merge pull request #1053 from code-corps/1042-github-installations

Standardize error output for GitHub.Event.Installation

Author: joshsmith

lib/code_corps/github/event/common/result_aggregator.ex

@@ -0,0 +1,36 @@
+defmodule CodeCorps.GitHub.Event.Common.ResultAggregator do
+  @moduledoc ~S"""
+  Module used for the purpose of aggregating results from multiple repository commit actions.
+  """
+
+  alias Ecto.Changeset
+
+  @doc ~S"""
+  Aggregates a list of database commit results into an `:ok`, or `:error` tuple.
+
+  All list members are assumed to be either an `{:ok, commited_record}` or
+  `{:error, changeset}`.
+
+  The aggregate is an `{:ok, commited_records}` if all results are
+  `{:ok, committed_record}`, or an `{:error, {commited_records, changesets}}` if
+  any of the results is an `{:error, changeset}`.
+  """
+  @spec aggregate(list) :: {:ok, list} | {:error, {list, list}}
+  def aggregate(results) when is_list(results) do
+    results |> collect() |> summarize()
+  end
+
+  @spec collect(list, list, list) :: tuple
+  defp collect(results, recods \\ [], changesets \\ [])
+  defp collect([{:ok, record} | tail], records, changesets) do
+    collect(tail, records ++ [record], changesets)
+  end
+  defp collect([{:error, %Changeset{} = changeset} | tail], records, changesets) do
+    collect(tail, records, changesets ++ [changeset])
+  end
+  defp collect([], records, changesets), do: {records, changesets}
+
+  @spec summarize(tuple) :: tuple
+  defp summarize({records, []}), do: {:ok, records}
+  defp summarize({records, changesets}), do: {:error, {records, changesets}}
+end

lib/code_corps/github/event/installation.ex

@@ -1,80 +1,114 @@
 defmodule CodeCorps.GitHub.Event.Installation do
   @moduledoc """
-  In charge of dealing with "Installation" GitHub Webhook events
+  In charge of handling a GitHub Webhook payload for the Installation event type
+  [https://developer.github.com/v3/activity/events/types/#installationevent](https://developer.github.com/v3/activity/events/types/#installationevent)
   """
 
+  @behaviour CodeCorps.GitHub.Event.Handler
+
   alias CodeCorps.{
     GithubAppInstallation,
     GithubEvent,
-    GitHub.Event.Installation.UnmatchedUser,
-    GitHub.Event.Installation.MatchedUser,
-    GitHub.Event.Installation.Repos,
-    GitHub.Event.Installation.Validator,
+    GitHub.Event.Installation,
     Repo,
     User
   }
 
-  @typep outcome :: {:ok, GithubAppInstallation.t, Task.t} | {:error, any}
-
-  @doc """
-  Handles an "Installation" GitHub Webhook event. The event could be
-  of subtype "created" or "deleted". Only the "created" variant is handled at
-  the moment.
+  alias Ecto.{Changeset, Multi}
 
-  `Installation::created` will first try to find the `User` using information
-  from the payload.
+  @type outcome :: {:ok, GithubAppInstallation.t} |
+                    {:error, :not_yet_implemented} |
+                    {:error, :unexpected_action} |
+                    {:error, :unexpected_payload} |
+                    {:error, :validation_error_on_syncing_installation} |
+                    {:error, :multiple_unprocessed_installations_found} |
+                    {:error, :github_api_error_on_syncing_repos} |
+                    {:error, :validation_error_on_deleting_removed_repos} |
+                    {:error, :validation_error_on_syncing_existing_repos} |
+                    {:error, :validation_error_on_marking_installation_processed}
 
-  Depending on the outcame of that operation, it will either call one of
+  @doc """
+  Handles the "Installation" GitHub Webhook event.
 
-  - `CodeCorps.GitHub.Event.Installation.UnmatchedUser.handle/2`
-  - `CodeCorps.GitHub.Event.Installation.MatchedUser.handle/2`
+  The event could be of subtype `created` or `deleted`. Only the `created`
+  variant is handled at the moment.
 
-  These helper modules will create or update the `GithubAppInstallation` with
-  proper data.
+  The process of handling the "created" event subtype is as follows
 
-  Once that is done, the outcome will be returned.
+  - try to match the sender with an existing `CodeCorps.User`
+  - call specific matching module depending on the user being matched or not
+    - `CodeCorps.GitHub.Event.Installation.MatchedUser.handle/2`
+    - `CodeCorps.GitHub.Event.Installation.UnmatchedUser.handle/1`
+  - sync installation repositories using a third modules
+    - `CodeCorps.GitHub.Event.Installation.Repos.process/1`
 
-  Additionally, a background task will launch
-  `CodeCorps.GitHub.Event.Installation.Repos.process_async` to asynchronously
-  fetch and process repositories for the installation.
+  If everything goes as expected, an `:ok` tuple will be returned, with a
+  `CodeCorps.GithubAppInstallation`, marked as "processed".
 
-  The installation will initially be returned with the state "processing", but
-  in the background, it will eventually switch to either "processed" or
-  "errored".
+  If a step in the process failes, an `:error` tuple will be returned, where the
+  second element is an atom indicating which step of the process failed.
   """
   @spec handle(GithubEvent.t, map) :: outcome
-  def handle(%GithubEvent{action: "created"}, payload) do
-    case payload |> Validator.valid? do
-      true -> payload |> do_handle() |> postprocess()
-      false -> {:error, :unexpected_payload}
-    end
-  end
-  def handle(%GithubEvent{action: "deleted"}, _) do
-    {:error, :not_fully_implemented}
+  def handle(%GithubEvent{}, payload) do
+    Multi.new
+    |> Multi.run(:payload, fn _ -> payload |> validate_payload() end)
+    |> Multi.run(:action, fn _ -> payload |> validate_action() end)
+    |> Multi.run(:user, fn _ -> payload |> find_user() end)
+    |> Multi.run(:installation, fn %{user: user} -> install_for_user(user, payload) end)
+    |> Multi.merge(&process_repos/1)
+    |> Repo.transaction
+    |> marshall_result()
   end
-  def handle(%GithubEvent{action: _action}, _payload) do
-    {:error, :unexpected_action}
+
+  @spec find_user(map) :: {:ok, User.t} | {:ok, nil} | {:error, :unexpected_user_payload}
+  defp find_user(%{"sender" => %{"id" => github_id}}) do
+    user = Repo.get_by(User, github_id: github_id)
+    {:ok, user}
   end
+  defp find_user(_), do: {:error, :unexpected_user_payload}
 
-  @spec do_handle(map) :: outcome
-  defp do_handle(%{"sender" => sender_attrs} = payload) do
-    case sender_attrs |> find_user() do
-      # No user was found with the specified github_id
-      nil -> UnmatchedUser.handle(payload)
-      # A user was found, matching the sspecified github_id
-      %User{} = user -> MatchedUser.handle(user, payload)
-    end
+  @spec install_for_user(User.t, map) :: outcome
+  defp install_for_user(%User{} = user, payload) do
+    Installation.MatchedUser.handle(user, payload)
+  end
+  defp install_for_user(nil, payload) do
+    Installation.UnmatchedUser.handle(payload)
   end
 
-  @spec postprocess({:ok, GithubAppInstallation.t} | {:error, any}) :: {:ok, GithubAppInstallation.t} | {:error, any}
-  defp postprocess({:ok, %GithubAppInstallation{} = installation}) do
+  @spec process_repos(%{installation: GithubAppInstallation.t}) :: Multi.t
+  defp process_repos(%{installation: %GithubAppInstallation{} = installation}) do
     installation
     |> Repo.preload(:github_repos)
-    |> Repos.process_async
+    |> Installation.Repos.process
+  end
+
+  @spec marshall_result(tuple) :: tuple
+  defp marshall_result({:ok, %{processed_installation: installation}}), do: {:ok, installation}
+  defp marshall_result({:error, :payload, :invalid, _steps}), do: {:error, :unexpected_payload}
+  defp marshall_result({:error, :action, :unexpected_action, _steps}), do: {:error, :unexpected_action}
+  defp marshall_result({:error, :action, :not_yet_implemented, _steps}), do: {:error, :not_yet_implemented}
+  defp marshall_result({:error, :user, :unexpected_user_payload, _steps}), do: {:error, :unexpected_payload}
+  defp marshall_result({:error, :installation, :unexpected_installation_payload, _steps}), do: {:error, :unexpected_payload}
+  defp marshall_result({:error, :installation, %Changeset{}, _steps}), do: {:error, :validation_error_on_syncing_installation}
+  defp marshall_result({:error, :installation, :too_many_unprocessed_installations, _steps}), do: {:error, :multiple_unprocessed_installations_found}
+  defp marshall_result({:error, :api_response, %CodeCorps.GitHub.APIError{}, _steps}), do: {:error, :github_api_error_on_syncing_repos}
+  defp marshall_result({:error, :deleted_repos, {_results, _changesets}, _steps}), do: {:error, :validation_error_on_deleting_removed_repos}
+  defp marshall_result({:error, :synced_repos, {_results, _changesets}, _steps}), do: {:error, :validation_error_on_syncing_existing_repos}
+  defp marshall_result({:error, :processed_installation, %Changeset{}, _steps}), do: {:error, :validation_error_on_marking_installation_processed}
+  defp marshall_result({:error, _errored_step, _error_response, _steps}), do: {:error, :unexpected_transaction_outcome}
+
+  @spec validate_payload(map) :: {:ok, :valid} | {:error, :invalid}
+  defp validate_payload(%{} = payload) do
+    case payload |> Installation.Validator.valid? do
+      true -> {:ok, :valid}
+      false -> {:error, :invalid}
+    end
   end
-  defp postprocess({:error, error}), do: {:error, error}
 
-  @spec find_user(any) :: User.t | nil
-  defp find_user(%{"id" => github_id}), do: User |> Repo.get_by(github_id: github_id)
-  defp find_user(_), do: :unexpected_user_payload
+  @spec validate_action(map) :: {:ok, :implemented} |
+                                {:error, :not_yet_implemented} |
+                                {:error, :unexpected_action}
+  defp validate_action(%{"action" => "created"}), do: {:ok, :implemented}
+  defp validate_action(%{"action" => "deleted"}), do: {:error, :not_yet_implemented}
+  defp validate_action(%{}), do: {:error, :unexpected_action}
 end

lib/code_corps/github/event/installation/matched_user.ex

@@ -13,7 +13,7 @@ defmodule CodeCorps.GitHub.Event.Installation.MatchedUser do
   }
 
   @typep process_outcome :: {:ok, GithubAppInstallation.t} | {:error, Changeset.t}
-  @typep outcome :: process_outcome | {:error, :too_many_unprocessed_installations}
+  @type outcome :: process_outcome | {:error, :too_many_unprocessed_installations}
 
   @doc """
   Handles the installation event in the case of a matched user.

lib/code_corps/github/event/installation/repos.ex

@@ -8,81 +8,55 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do
     GithubRepo,
     GitHub.Installation,
     GitHub,
+    GitHub.Event.Common.ResultAggregator,
     Repo
   }
 
   alias CodeCorps.GitHub.Adapters.GithubRepo, as: GithubRepoAdapter
 
   alias Ecto.{Changeset, Multi}
 
-  @typep repo_result :: {:ok | :error, GithubRepo.t | Changeset.t}
-  @typep aggregated_result :: {:ok | :error, list(GithubRepo.t | Changeset.t)}
+  @typep aggregated_result :: {:ok, list(GithubRepo.t)} |
+                              {:error, {list(GithubRepo.t), list(Changeset.t)}}
 
   @doc ~S"""
-  Marks a `GithubAppInstallation` as "processing" and immediately returns it.
+  Creates and returns a multi used to sync a `CodeCorps.GithubRepo` records
+  associated with the provided `CodeCorps.GithubAppInstallation` record, based
+  on freshly retrieved GitHub data.
 
-  In the background, it fires a task to asynchronously call &process/1
-  """
-  @spec process_async(GithubAppInstallation.t) :: {:ok, GithubAppInstallation.t, Task.t}
-  def process_async(%GithubAppInstallation{} = installation) do
-    {:ok, %GithubAppInstallation{} = updated_installation} =
-      installation |> set_state("processing")
-
-    repo_processing_task = Task.Supervisor.async(
-      :background_processor, fn -> updated_installation |> process() end)
+  Note that a GitHub API call is being called as part of this process.
 
-    {:ok, {updated_installation, repo_processing_task}}
-  end
-
-  @doc ~S"""
-  Fetches a list of repositories for a `GithubAppInstallation` from the GitHub
-  API and matches up `GithubRepo` records stored locally using that list as the
-  master list.
+  The list of repositories from the API call is considered the master list.
+  - Anything existing locally, but not on this list is deleted.
+  - Anything existing both locally and on this list is updated.
+  - Anything existing not locally, but on this list is created.
   """
-  @spec process(GithubAppInstallation.t) :: {:ok | :error, GithubAppInstallation.t}
+  @spec process(GithubAppInstallation.t) :: Multi.t
   def process(%GithubAppInstallation{} = installation) do
-    multi =
-      Multi.new
-      |> Multi.run(:processing_installation, fn _ -> {:ok, installation} end)
-      |> Multi.run(:api_response, &fetch_api_repo_list/1)
-      |> Multi.run(:repo_attrs_list, &adapt_api_repo_list/1)
-      |> Multi.run(:deleted_repos, &delete_repos/1)
-      |> Multi.run(:synced_repos, &sync_repos/1)
-      |> Multi.run(:processed_installation, &mark_processed/1)
-
-    case Repo.transaction(multi) do
-      {:ok, %{processed_installation: installation}} ->
-        {:ok, installation}
-      {:error, _errored_step, error_response, _steps} ->
-        {:ok, errored_installation} = installation |> set_state("errored")
-        {:error, errored_installation, error_response}
-    end
-  end
-
-  @spec set_state(GithubAppInstallation.t, String.t) :: {:ok, GithubAppInstallation.t}
-  defp set_state(%GithubAppInstallation{} = installation, state)
-    when state in ~w(processing processed errored) do
-
-    installation
-    |> Changeset.change(%{state: state})
-    |> Repo.update
+    Multi.new
+    |> Multi.run(:processing_installation, fn _ -> {:ok, installation} end)
+    |> Multi.run(:api_response, &fetch_api_repo_list/1)
+    |> Multi.run(:repo_attrs_list, &adapt_api_repo_list/1)
+    |> Multi.run(:deleted_repos, &delete_repos/1)
+    |> Multi.run(:synced_repos, &sync_repos/1)
+    |> Multi.run(:processed_installation, &mark_processed/1)
   end
 
   # transaction step 1
-  @spec fetch_api_repo_list(%{processing_installation: GithubAppInstallation.t}) :: {:ok, map} | {:error, GitHub.api_error_struct}
+  @spec fetch_api_repo_list(map) :: {:ok, map} | {:error, GitHub.api_error_struct}
   defp fetch_api_repo_list(%{processing_installation: %GithubAppInstallation{} = installation}) do
     installation |> Installation.repositories()
   end
 
   # transaction step 2
-  @spec adapt_api_repo_list(%{api_response: any}) :: {:ok, list(map)}
+  @spec adapt_api_repo_list(map) :: {:ok, list(map)}
   defp adapt_api_repo_list(%{api_response: repositories}) do
     adapter_results = repositories |> Enum.map(&GithubRepoAdapter.from_api/1)
     {:ok, adapter_results}
   end
 
   # transaction step 3
-  @spec delete_repos(%{processing_installation: GithubAppInstallation.t, repo_attrs_list: list(map)}) :: aggregated_result
+  @spec delete_repos(map) :: aggregated_result
   defp delete_repos(%{
     processing_installation: %GithubAppInstallation{github_repos: github_repos},
     repo_attrs_list: attrs_list}) when is_list(attrs_list) do
@@ -97,38 +71,25 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do
       end
     end)
     |> Enum.reject(&is_nil/1)
-    |> aggregate
+    |> ResultAggregator.aggregate
   end
 
   # transaction step 4
-  @spec sync_repos(%{processing_installation: GithubAppInstallation.t, repo_attrs_list: list(map)}) :: aggregated_result
+  @spec sync_repos(map) :: aggregated_result
   defp sync_repos(%{
     processing_installation: %GithubAppInstallation{} = installation,
     repo_attrs_list: attrs_list }) when is_list(attrs_list) do
 
     attrs_list
     |> Enum.map(&sync(installation, &1))
-    |> aggregate
-  end
-
-  @spec aggregate(list(repo_result)) :: aggregated_result
-  defp aggregate(results) do
-    case results |> Enum.filter(fn {state, _data} -> state == :error end) do
-      [] -> results |> reduce(:ok)
-      errors -> errors |> reduce(:error)
-    end
-  end
-
-  @spec reduce(list(repo_result), :ok | :error) :: aggregated_result
-  defp reduce(data, state) when state in [:ok, :error] do
-    data
-    |> Enum.map(&Tuple.to_list/1) # {:ok, repo} -> [:ok, repo]
-    |> Enum.map(&Enum.at(&1, 1)) # [:ok, repo] -> repo
-    |> (fn data -> {state, data} end).() # repos -> {:ok, repos}
+    |> ResultAggregator.aggregate
   end
 
   @spec sync(GithubAppInstallation.t, map) :: {:ok, GithubRepo.t}
-  defp sync(%GithubAppInstallation{github_repos: github_repos} = installation, %{github_id: github_id} = repo_attributes) do
+  defp sync(
+    %GithubAppInstallation{github_repos: github_repos} = installation,
+    %{github_id: github_id} = repo_attributes) do
+
     case github_repos |> Enum.find(fn %GithubRepo{} = gr -> gr.github_id == github_id end) do
       nil -> create(installation, repo_attributes)
       %GithubRepo{} = github_repo -> github_repo |> update(repo_attributes)
@@ -163,6 +124,8 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do
   # transaction step 5
   @spec mark_processed(%{processing_installation: GithubAppInstallation.t}) :: {:ok, GithubAppInstallation.t}
   defp mark_processed(%{processing_installation: %GithubAppInstallation{} = installation}) do
-    installation |> set_state("processed")
+    installation
+    |> Changeset.change(%{state: "processed"})
+    |> Repo.update
   end
 end

lib/code_corps/github/event/installation/unmatched_user.ex

@@ -1,5 +1,5 @@
 defmodule CodeCorps.GitHub.Event.Installation.UnmatchedUser do
-  @moduledoc """
+  @moduledoc ~S"""
   In charge of handling the unmatched user case of an installation event
   """
 
@@ -10,9 +10,9 @@ defmodule CodeCorps.GitHub.Event.Installation.UnmatchedUser do
   }
 
   @typep process_outcome :: {:ok, GithubAppInstallation.t} | {:error, Changeset.t}
-  @typep outcome :: process_outcome |  {:error, :unexpected_installation_payload}
+  @type outcome :: process_outcome |  {:error, :unexpected_installation_payload}
 
-  @doc """
+  @doc ~S"""
   Handles the installation event in the case of an unmatched user.
 
   This is done by attempting to find a `GithubAppInstallation` record by it's