Merge pull request #1057 from code-corps/1043-standardize-output-for-installation-repositories

Standardized output of InstallationRepositories webhook event handler

Author: joshsmith

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

@@ -8,12 +8,12 @@ defmodule CodeCorps.GitHub.Event.Common.ResultAggregator do
   @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
+  All list members are assumed to be either an `{:ok, committed_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}`.
+  The aggregate is an `{:ok, committed_records}` if all results are
+  `{:ok, committed_record}`, or an `{:error, {committed_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

lib/code_corps/github/event/installation_repositories.ex

@@ -1,89 +1,97 @@
 defmodule CodeCorps.GitHub.Event.InstallationRepositories do
-  @moduledoc """
-  In charge of dealing with "InstallationRepositories" GitHub Webhook events
+  @moduledoc ~S"""
+  In charge of handling a GitHub Webhook payload for the
+  InstallationRepositories event type.
+
+  [https://developer.github.com/v3/activity/events/types/#installationrepositoriesevent](https://developer.github.com/v3/activity/events/types/#installationrepositoriesevent)
   """
 
+  @behaviour CodeCorps.GitHub.Event.Handler
+
   alias CodeCorps.{
     GithubAppInstallation,
     GithubEvent,
     GithubRepo,
+    GitHub.Event.Common.ResultAggregator,
+    GitHub.Event.InstallationRepositories,
     Repo
   }
 
-  alias Ecto.Changeset
+  alias Ecto.{Changeset, Multi}
 
-  @typep outcome :: {:ok, [GithubRepo.t]} |
-                    {:error, :no_installation} |
-                    {:error, :unexpected_action_or_payload} |
-                    {:error, :unexpected_installation_payload} |
-                    {:error, :unexpected_repo_payload}
+  @type outcome :: {:ok, list(GithubRepo.t)} |
+                    {:error, :unmatched_installation} |
+                    {:error, :unexpected_action} |
+                    {:error, :unexpected_payload} |
+                    {:error, :validation_error_on_syncing_repos} |
+                    {:error, :unexpected_transaction_outcome}
 
   @doc """
   Handles an "InstallationRepositories" GitHub Webhook event. The event could be
   of subtype "added" or "removed" and is handled differently based on that.
 
-  `InstallationRepositories::added` will
-    - find `GithubAppInstallation`
-      - marks event as errored if not found
-      - marks event as errored if payload does not have keys it needs, meaning
-        there's something wrong with our code and we need to updated
-    - find or create `GithubRepo` records affected
+  - the process of handling the "added" subtype is as follows
+    - try to match with `CodeCorps.GithubAppInstallation` record
+    - sync affected `CodeCorps.GithubRepo` records (update, create)
 
-  `InstallationRepositories::removed` will
-    - find `GithubAppInstallation`
-      - marks event as errored if not found
-      - marks event as errored if payload does not have keys it needs, meaning
-        there's something wrong with our code and we need to updated
-    - find `GithubRepo` records affected and delete them
-      - if there is no `GithubRepo` to delete, it just skips it
-      - `ProjectGithubRepo` are deleted automatically, since they are set to
-        `on_delete: :delete_all`
+  - the process of handling the "removed" subtype is as follows
+    - try to match with a `CodeCorps.GithubAppInstallation` record
+    - delete affected `CodeCorps.GithubRepo` records, respecting the rules
+      - if the GitHub payload for a repo is not matched with a record in our
+        database, just skip deleting it
+      - if the deleted `CodeCorps.GithubRepo` record is associated with
+        `CodeCorps.ProjectGithubRepo` records, they are deleted automatically,
+        due to `on_delete: :delete_all` set at the database level.
   """
   @spec handle(GithubEvent.t, map) :: outcome
-  def handle(%GithubEvent{action: action}, payload), do: do_handle(action, payload)
-
-  @spec do_handle(String.t, map) :: outcome
-  defp do_handle("added", %{"installation" => installation_attrs, "repositories_added" => repositories_attr_list}) do
-    case installation_attrs |> find_installation() do
-      nil -> {:error, :no_installation}
-      :unexpected_installation_payload -> {:error, :unexpected_installation_payload}
-      %GithubAppInstallation{} = installation ->
-        case repositories_attr_list |> Enum.all?(&valid?/1) do
-          true -> find_or_create_all(installation, repositories_attr_list)
-          false -> {:error, :unexpected_repo_payload}
-        end
-    end
+  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(:installation, fn _ -> payload |> match_installation() end)
+    |> Multi.run(:repos, fn %{installation: installation} -> installation |> sync_repos(payload) end)
+    |> Repo.transaction
+    |> marshall_result()
   end
-  defp do_handle("removed", %{"installation" => installation_attrs, "repositories_removed" => repositories_attr_list}) do
-    case installation_attrs |> find_installation() do
-      nil -> {:error, :no_installation}
-      :unexpected_installation_payload -> {:error, :unexpected_installation_payload}
-      %GithubAppInstallation{} = installation ->
-        case repositories_attr_list |> Enum.all?(&valid?/1) do
-          true -> delete_all(installation, repositories_attr_list)
-          false -> {:error, :unexpected_repo_payload}
-        end
+
+  @spec validate_payload(map) :: {:ok, :valid} | {:error, :invalid}
+  defp validate_payload(%{} = payload) do
+    case payload |> InstallationRepositories.Validator.valid? do
+      true -> {:ok, :valid}
+      false -> {:error, :invalid}
     end
   end
-  defp do_handle(_action, _payload), do: {:error, :unexpected_action_or_payload}
 
-  @spec find_installation(any) :: GithubAppInstallation.t | nil | :unexpected_installation_payload
-  defp find_installation(%{"id" => github_id}), do: GithubAppInstallation |> Repo.get_by(github_id: github_id)
-  defp find_installation(_payload), do: :unexpected_installation_payload
+  @valid_actions ~w(added removed)
+  @spec validate_action(map) :: {:ok, :implemented} | {:error, :unexpected_action}
+  defp validate_action(%{"action" => action}) when action in @valid_actions, do: {:ok, :implemented}
+  defp validate_action(%{}), do: {:error, :unexpected_action}
 
-  # should return true if the payload is a map and has the expected keys
-  @spec valid?(any) :: boolean
-  defp valid?(%{"id" => _, "name" => _}), do: true
-  defp valid?(_), do: false
+  @spec match_installation(map) :: {:ok, GithubAppInstallation.t} |
+                                   {:error, :unmatched_installation}
+  defp match_installation(%{"installation" => %{"id" => github_id}}) do
+    case GithubAppInstallation |> Repo.get_by(github_id: github_id) do
+      nil -> {:error, :unmatched_installation}
+      %GithubAppInstallation{} = installation -> {:ok, installation}
+    end
+  end
+  defp match_installation(%{}), do: {:error, :unmatched_installation}
 
-  @spec find_or_create_all(GithubAppInstallation.t, list(map)) :: {:ok, list(GithubRepo.t)}
-  defp find_or_create_all(%GithubAppInstallation{} = installation, repositories_attr_list) when is_list(repositories_attr_list) do
-    repositories_attr_list
+  @spec sync_repos(GithubAppInstallation.t, map) :: {:ok, list(GithubRepo.t)} | {:error, {list(GithubRepo.t), list(Changeset.t)}}
+  defp sync_repos(%GithubAppInstallation{} = installation, %{"action" => "added", "repositories_added" => repositories}) do
+    repositories
     |> Enum.map(&find_or_create(installation, &1))
-    |> aggregate()
+    |> ResultAggregator.aggregate
+  end
+  defp sync_repos(%GithubAppInstallation{} = installation, %{"action" => "removed", "repositories_removed" => repositories}) do
+    repositories
+    |> Enum.map(&find_repo(installation, &1))
+    |> Enum.reject(&is_nil/1)
+    |> Enum.map(&Repo.delete/1)
+    |> ResultAggregator.aggregate
   end
 
-  @spec find_or_create(GithubAppInstallation.t, map) :: {:ok, GithubRepo.t}
+  @spec find_or_create(GithubAppInstallation.t, map) :: {:ok, GithubRepo.t} | {:error, Changeset.t}
   defp find_or_create(%GithubAppInstallation{} = installation, %{"id" => github_id, "name" => name} = attrs) do
     case find_repo(installation, attrs) do
       nil ->
@@ -92,32 +100,23 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
         |> Changeset.put_assoc(:github_app_installation, installation)
         |> Repo.insert()
       %GithubRepo{} = github_repo ->
-        {:ok, github_repo}
+        github_repo
+        |> Changeset.change(%{name: name})
+        |> Repo.update()
     end
   end
 
-  @spec delete_all(GithubAppInstallation.t, list(map)) :: {:ok, [GithubRepo.t]}
-  defp delete_all(%GithubAppInstallation{} = installation, repositories_attr_list) when is_list(repositories_attr_list) do
-    repositories_attr_list
-    |> Enum.map(&find_repo(installation, &1))
-    |> Enum.reject(&is_nil/1)
-    |> Enum.map(&Repo.delete/1)
-    |> aggregate()
-  end
-
   @spec find_repo(GithubAppInstallation.t, map) :: GithubRepo.t | nil
   defp find_repo(%GithubAppInstallation{id: installation_id}, %{"id" => github_id}) do
     GithubRepo
     |> Repo.get_by(github_app_installation_id: installation_id, github_id: github_id)
   end
 
-  # [{:ok, repo_1}, {:ok, repo_2}, {:ok, repo_3}] -> {:ok, [repo_1, repo_2, repo_3]}
-  @spec aggregate(list({:ok, GithubRepo.t})) :: {:ok, list(GithubRepo.t)}
-  defp aggregate(results) do
-    repositories =
-      results
-      |> Enum.map(fn {:ok, %GithubRepo{} = github_repo} -> github_repo end)
-
-    {:ok, repositories}
-  end
+  @spec marshall_result(tuple) :: tuple
+  defp marshall_result({:ok, %{repos: repos}}), do: {:ok, repos}
+  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, :installation, :unmatched_installation, _steps}), do: {:error, :unmatched_installation}
+  defp marshall_result({:error, :repos, {_repos, _changesets}, _steps}), do: {:error, :validation_error_on_syncing_repos}
+  defp marshall_result({:error, _errored_step, _error_response, _steps}), do: {:error, :unexpected_transaction_outcome}
 end

lib/code_corps/github/event/installation_repositories/validator.ex

@@ -0,0 +1,37 @@
+defmodule CodeCorps.GitHub.Event.InstallationRepositories.Validator do
+  @moduledoc ~S"""
+  In charge of validatng a GitHub InstallationRepositories webhook payload.
+
+  [https://developer.github.com/v3/activity/events/types/#installationrepositoriesevent](https://developer.github.com/v3/activity/events/types/#installationrepositoriesevent)
+  """
+
+  @doc ~S"""
+  Returns `true` if all keys required to properly handle an
+  InstallationRepositories webhook are present in the provided payload.
+  """
+  @spec valid?(map) :: boolean
+  def valid?(%{
+    "action" => _, "installation" => %{"id" => _},
+    "repositories_added" => added, "repositories_removed" => removed})
+    when is_list(added) when is_list(removed) do
+
+    (added ++ removed) |> Enum.all?(&repository_valid?/1)
+  end
+  def valid?(%{
+    "action" => _, "installation" => %{"id" => _},
+    "repositories_added" => added}) when is_list(added) do
+
+    added |> Enum.all?(&repository_valid?/1)
+  end
+  def valid?(%{
+    "action" => _, "installation" => %{"id" => _},
+    "repositories_removed" => removed}) when is_list(removed) do
+
+    removed |> Enum.all?(&repository_valid?/1)
+  end
+  def valid?(_), do: false
+
+  @spec repository_valid?(any) :: boolean
+  defp repository_valid?(%{"id" => _, "name" => _}), do: true
+  defp repository_valid?(_), do: false
+end

test/lib/code_corps/github/event/installation_repositories/validator_test.exs

@@ -0,0 +1,28 @@
+defmodule CodeCorps.GitHub.Event.InstallationRepositories.ValidatorTest do
+  @moduledoc false
+
+  use ExUnit.Case, async: true
+
+  import CodeCorps.GitHub.TestHelpers
+
+  alias CodeCorps.GitHub.Event.InstallationRepositories.Validator
+
+  describe "valid?/1" do
+    test "returns true for any Issues event fixture" do
+      assert "installation_repositories_added" |> load_event_fixture() |> Validator.valid?
+      assert "installation_repositories_removed" |> load_event_fixture() |> Validator.valid?
+    end
+
+    test "returns false for an unsupported structure" do
+      refute Validator.valid?("foo")
+      refute Validator.valid?(%{"action" => "foo", "foo" => "bar"})
+      refute Validator.valid?(%{"action" => "foo", "installation" => %{"bar" => "baz"}})
+      refute Validator.valid?(%{"action" => "added", "installation" => %{"id" => "foo"}, "repositories_added" => [%{"id" => "foo"}]})
+      refute Validator.valid?(%{"action" => "removed", "installation" => %{"id" => "foo"}, "repositories_removed" => [%{"id" => "ba"}]})
+      refute Validator.valid?(%{"action" => "added", "installation" => %{"id" => "foo"}, "repositories_added" => ["foo"]})
+      refute Validator.valid?(%{"action" => "removed", "installation" => %{"id" => "foo"}, "repositories_removed" => ["bar"]})
+      refute Validator.valid?(%{"action" => "added", "installation" => %{"id" => "foo"}, "repositories_added" => "foo"})
+      refute Validator.valid?(%{"action" => "removed", "installation" => %{"id" => "foo"}, "repositories_removed" => "bar"})
+    end
+  end
+end

test/lib/code_corps/github/event/installation_repositories_test.exs

@@ -13,16 +13,19 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositoriesTest do
   }
 
   describe "handle/2" do
+    @payload load_event_fixture("installation_repositories_added")
+
     test "marks event as errored if invalid action" do
-      payload = %{}
+      payload = @payload |> Map.put("action", "foo")
       event = build(:github_event, action: "foo", type: "installation_repositories")
-      assert {:error, :unexpected_action_or_payload} == InstallationRepositories.handle(event, payload)
+      assert {:error, :unexpected_action} == InstallationRepositories.handle(event, payload)
     end
 
     test "marks event as errored if invalid payload" do
-      payload = %{}
       event = build(:github_event, action: "added", type: "installation_repositories")
-      assert {:error, :unexpected_action_or_payload} == InstallationRepositories.handle(event, payload)
+      payload = @payload |> Map.delete("action")
+      assert {:error, :unexpected_payload} ==
+        InstallationRepositories.handle(event, payload)
     end
   end
 
@@ -76,18 +79,18 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositoriesTest do
 
     test "marks event as errored if invalid instalation payload" do
       event = insert(:github_event, action: "added", type: "installation_repositories")
-      assert {:error, :unexpected_installation_payload} == InstallationRepositories.handle(event, @payload |> Map.put("installation", "foo"))
+      assert {:error, :unexpected_payload} == InstallationRepositories.handle(event, @payload |> Map.put("installation", "foo"))
     end
 
     test "marks event as errored if invalid repo payload" do
       event = insert(:github_event, action: "added", type: "installation_repositories")
       insert(:github_app_installation, github_id: @payload["installation"]["id"])
-      assert {:error, :unexpected_repo_payload} == InstallationRepositories.handle(event, @payload |> Map.put("repositories_added", ["foo"]))
+      assert {:error, :unexpected_payload} == InstallationRepositories.handle(event, @payload |> Map.put("repositories_added", ["foo"]))
     end
 
     test "marks event as errored if no installation" do
       event = insert(:github_event, action: "added", type: "installation_repositories")
-      assert {:error, :no_installation} == InstallationRepositories.handle(event, @payload)
+      assert {:error, :unmatched_installation} == InstallationRepositories.handle(event, @payload)
     end
   end
 
@@ -131,18 +134,22 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositoriesTest do
 
     test "marks event as errored if invalid instalation payload" do
       event = build(:github_event, action: "removed", type: "installation_repositories")
-      assert {:error, :unexpected_installation_payload} == InstallationRepositories.handle(event, @payload |> Map.put("installation", "foo"))
+      payload = @payload |> Map.put("installation", "foo")
+      assert {:error, :unexpected_payload} ==
+        InstallationRepositories.handle(event, payload)
     end
 
     test "marks event as errored if invalid repo payload" do
       event = build(:github_event, action: "removed", type: "installation_repositories")
       insert(:github_app_installation, github_id: @payload["installation"]["id"])
-      assert {:error, :unexpected_repo_payload} == InstallationRepositories.handle(event, @payload |> Map.put("repositories_removed", ["foo"]))
+      payload = @payload |> Map.put("repositories_removed", ["foo"])
+      assert {:error, :unexpected_payload} ==
+        InstallationRepositories.handle(event, payload)
     end
 
     test "marks event as errored if no installation" do
       event = build(:github_event, action: "added", type: "installation_repositories")
-      assert {:error, :no_installation} == InstallationRepositories.handle(event, @payload)
+      assert {:error, :unmatched_installation} == InstallationRepositories.handle(event, @payload)
     end
   end
 end