Merge pull request #1047 from code-corps/1044-standardize-issues-webhook

Standardize output of CodeCorps.GitHub.Events.Issues

Author: joshsmith

README.md

@@ -6,7 +6,6 @@
 [![Deps Status](https://beta.hexfaktor.org/badge/prod/github/code-corps/code-corps-api.svg)](https://beta.hexfaktor.org/github/code-corps/code-corps-api)
 [![Deps Status](https://beta.hexfaktor.org/badge/all/github/code-corps/code-corps-api.svg)](https://beta.hexfaktor.org/github/code-corps/code-corps-api)
 [![Slack Status](http://slack.codecorps.org/badge.svg)](http://slack.codecorps.org)
-</p>
 
 ---
 

lib/code_corps/github/event/handler.ex

@@ -0,0 +1,16 @@
+defmodule CodeCorps.GitHub.Event.Handler do
+  @moduledoc ~S"""
+  Default behavior for all GitHub webhook event handlers.
+  """
+
+  alias CodeCorps.GithubEvent
+
+  @doc ~S"""
+  The only entry point a GitHub webhook event handler function should contain.
+
+  Receives a `CodeCorps.GithubEvent` record and a payload, returns an `:ok`
+  tuple if the process was successful, or an `:error` tuple, where the second
+  element is an atom, if it failed.
+  """
+  @callback handle(GithubEvent.t, map) :: {:ok, any} | {:error, atom}
+end

lib/code_corps/github/event/issues.ex

@@ -1,10 +1,12 @@
 defmodule CodeCorps.GitHub.Event.Issues do
   @moduledoc ~S"""
-  In charge of dealing with "Issues" GitHub Webhook events
+  In charge of handling a GitHub Webhook payload for the Issues event type
 
-  https://developer.github.com/v3/activity/events/types/#issuesevent
+  [https://developer.github.com/v3/activity/events/types/#issuesevent](https://developer.github.com/v3/activity/events/types/#issuesevent)
   """
 
+  @behaviour CodeCorps.GitHub.Event.Handler
+
   alias CodeCorps.{
     GithubEvent,
     GitHub.Event.Common.RepoFinder,
@@ -16,63 +18,70 @@ defmodule CodeCorps.GitHub.Event.Issues do
   }
   alias Ecto.Multi
 
-  @typep outcome :: {:ok, list(Task.t)} |
-                    {:error, :not_fully_implemented} |
-                    {:error, :unexpected_payload} |
-                    {:error, :unexpected_action} |
-                    {:error, :unmatched_repository}
-
-  @implemented_actions ~w(opened closed edited reopened)
-  @unimplemented_actions ~w(assigned unassigned milestoned demilestoned labeled unlabeled)
+  @type outcome :: {:ok, list(Task.t)} |
+                   {:error, :not_fully_implemented} |
+                   {:error, :unexpected_action} |
+                   {:error, :unexpected_payload} |
+                   {:error, :repository_not_found} |
+                   {:error, :validation_error_on_inserting_user} |
+                   {:error, :multiple_github_users_matched_same_cc_user} |
+                   {:error, :validation_error_on_syncing_tasks} |
+                   {:error, :unexpected_transaction_outcome}
 
   @doc ~S"""
   Handles the "Issues" GitHub webhook
 
   The process is as follows
   - validate the payload is structured as expected
-  - try and find the appropriate `GithubRepo` record.
-  - for each `ProjectGithubRepo` belonging to that `Project`
-    - find or initialize a new `Task`
-    - try and find a `User`, associate `Task` with user
-    - commit the change as an insert or update action
+  - validate the action is properly supported
+  - match payload with affected `CodeCorps.GithubRepo` record using `CodeCorps.GitHub.Event.Common.RepoFinder`
+  - match with a `CodeCorps.User` using `CodeCorps.GitHub.Event.Issues.UserLinker`
+  - for each `CodeCorps.ProjectGithubRepo` belonging to matched repo
+    - match and update, or create a `CodeCorps.Task` on the associated `CodeCorps.Project`
 
-  Depending on the success of the process, the function will return one of
-  - `{:ok, list_of_tasks}`
-  - `{:error, :not_fully_implemented}` - while we're aware of this action, we have not implemented support for it yet
-  - `{:error, :unexpected_payload}` - the payload was not as expected
-  - `{:error, :unexpected_action}` - the action was not of type we are aware of
-  - `{:error, :unmatched_repository}` - the repository for this issue was not found
+  If the process runs all the way through, the function will return an `:ok`
+  tuple with a list of affected (created or updated) tasks.
 
-  Note that it is also possible to have a matched GithubRepo, but with that
-  record not having any ProjectGithubRepo children. The outcome of that case
-  should NOT be an errored event, since it simply means that the GithubRepo
-  was not linked to a Project by the Project owner. This is allowed and
-  relatively common.
+  If it fails, it will instead return an `:error` tuple, where the second
+  element is the atom indicating a reason.
   """
   @spec handle(GithubEvent.t, map) :: outcome
-  def handle(%GithubEvent{action: action}, payload) when action in @implemented_actions do
-    case payload |> Validator.valid? do
-      true -> do_handle(payload)
-      false -> {:error, :unexpected_payload}
-    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(:repo, fn _ -> RepoFinder.find_repo(payload) end)
+    |> Multi.run(:user, fn _ -> UserLinker.find_or_create_user(payload) end)
+    |> Multi.run(:tasks, fn %{repo: github_repo, user: user} -> TaskSyncer.sync_all(github_repo, user, payload) end)
+    |> Repo.transaction
+    |> marshall_result()
   end
-  def handle(%GithubEvent{action: action}, _payload) when action in @unimplemented_actions do
-    {:error, :not_fully_implemented}
-  end
-  def handle(%GithubEvent{action: _action}, _payload), do: {:error, :unexpected_action}
 
-  @spec do_handle(map) :: {:ok, list(Task.t)} | {:error, :unmatched_repository}
-  defp do_handle(%{} = payload) do
-    multi =
-      Multi.new
-      |> Multi.run(:repo, fn _ -> RepoFinder.find_repo(payload) end)
-      |> Multi.run(:user, fn _ -> UserLinker.find_or_create_user(payload) end)
-      |> Multi.run(:tasks, fn %{repo: github_repo, user: user} -> TaskSyncer.sync_all(github_repo, user, payload) end)
+  @spec marshall_result(tuple) :: tuple
+  defp marshall_result({:ok, %{tasks: tasks}}), do: {:ok, tasks}
+  defp marshall_result({:error, :payload, :invalid, _steps}), do: {:error, :unexpected_payload}
+  defp marshall_result({:error, :action, :not_fully_implemented, _steps}), do: {:error, :not_fully_implemented}
+  defp marshall_result({:error, :action, :unexpected_action, _steps}), do: {:error, :unexpected_action}
+  defp marshall_result({:error, :repo, :unmatched_project, _steps}), do: {:ok, []}
+  defp marshall_result({:error, :repo, :unmatched_repository, _steps}), do: {:error, :repository_not_found}
+  defp marshall_result({:error, :user, %Ecto.Changeset{}, _steps}), do: {:error, :validation_error_on_inserting_user}
+  defp marshall_result({:error, :user, :multiple_users, _steps}), do: {:error, :multiple_github_users_matched_same_cc_user}
+  defp marshall_result({:error, :tasks, {_tasks, _errors}, _steps}), do: {:error, :validation_error_on_syncing_tasks}
+  defp marshall_result({:error, _errored_step, _error_response, _steps}), do: {:error, :unexpected_transaction_outcome}
+
+  @implemented_actions ~w(opened closed edited reopened)
+  @unimplemented_actions ~w(assigned unassigned milestoned demilestoned labeled unlabeled)
+
+  @spec validate_action(map) :: {:ok, :implemented} | {:error, :not_fully_implemented | :unexpected_action}
+  defp validate_action(%{"action" => action}) when action in @implemented_actions, do: {:ok, :implemented}
+  defp validate_action(%{"action" => action}) when action in @unimplemented_actions, do: {:error, :not_fully_implemented}
+  defp validate_action(_payload), do: {:error, :unexpected_action}
 
-    case Repo.transaction(multi) do
-      {:ok, %{tasks: tasks}} -> {:ok, tasks}
-      {:error, :repo, :unmatched_project, _steps} -> {:ok, []}
-      {:error, _errored_step, error_response, _steps} -> {:error, error_response}
+  @spec validate_payload(map) :: {:ok, :valid} | {:error, :invalid}
+  defp validate_payload(%{} = payload) do
+    case payload |> Validator.valid? do
+      true -> {:ok, :valid}
+      false -> {:error, :invalid}
     end
   end
 end

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

@@ -11,6 +11,7 @@ defmodule CodeCorps.GitHub.Event.Issues.Validator do
   """
   @spec valid?(map) :: boolean
   def valid?(%{
+    "action" => _,
     "issue" => %{
       "id" => _, "title" => _, "body" => _, "state" => _,
       "user" => %{"id" => _}

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

@@ -14,7 +14,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
   }
 
   describe "handle/2" do
-    @payload load_event_fixture("issues_opened")
+    @payload load_event_fixture("issues_opened") |> Map.put("action", "foo")
 
     test "returns error if action of the event is wrong" do
       event = build(:github_event, action: "foo", type: "issues")
@@ -83,7 +83,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
     end
 
     test "with unmatched user, returns error if unmatched repository" do
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
       refute Repo.one(User)
     end
 
@@ -142,7 +142,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
       %{"issue" => %{"user" => %{"id" => user_github_id}}} = @payload
       insert(:user, github_id: user_github_id)
 
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
     end
 
     test "returns error if payload is wrong" do
@@ -217,7 +217,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
     end
 
     test "with unmatched user, returns error if unmatched repository" do
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
       refute Repo.one(User)
     end
 
@@ -276,7 +276,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
       %{"issue" => %{"user" => %{"id" => user_github_id}}} = @payload
       insert(:user, github_id: user_github_id)
 
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
     end
 
     test "returns error if payload is wrong" do
@@ -351,7 +351,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
     end
 
     test "with unmatched user, returns error if unmatched repository" do
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
       refute Repo.one(User)
     end
 
@@ -410,7 +410,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
       %{"issue" => %{"user" => %{"id" => user_github_id}}} = @payload
       insert(:user, github_id: user_github_id)
 
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
     end
 
     test "returns error if payload is wrong" do
@@ -485,7 +485,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
     end
 
     test "with unmatched user, returns error if unmatched repository" do
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
       refute Repo.one(User)
     end
 
@@ -544,7 +544,7 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
       %{"issue" => %{"user" => %{"id" => user_github_id}}} = @payload
       insert(:user, github_id: user_github_id)
 
-      assert Issues.handle(@event, @payload) == {:error, :unmatched_repository}
+      assert Issues.handle(@event, @payload) == {:error, :repository_not_found}
     end
 
     test "returns error if payload is wrong" do
@@ -560,57 +560,24 @@ defmodule CodeCorps.GitHub.Event.IssuesTest do
     end
   end
 
-  describe "handle/2 for Issues::assigned" do
-    @payload %{}
-
-    test "is not implemented" do
-      event = build(:github_event, action: "assigned", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
-    end
-  end
-
-  describe "handle/2 for Issues::unassigned" do
-    @payload %{}
-
-    test "is not implemented" do
-      event = build(:github_event, action: "unassigned", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
-    end
-  end
-
-  describe "handle/2 for Issues::labeled" do
-    @payload %{}
-
-    test "is not implemented" do
-      event = build(:github_event, action: "labeled", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
-    end
-  end
-
-  describe "handle/2 for Issues::unlabeled" do
-    @payload %{}
-
-    test "is not implemented" do
-      event = build(:github_event, action: "unlabeled", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
-    end
-  end
-
-  describe "handle/2 for Issues::milestoned" do
-    @payload %{}
+  @unimplemented_actions ~w(assigned unassigned labeled unlabeled milestoned demilestoned)
 
-    test "is not implemented" do
-      event = build(:github_event, action: "milestoned", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
-    end
-  end
+  @unimplemented_actions |> Enum.each(fn action ->
+    describe "handle/2 for Issues::#{action}" do
+      @payload %{
+        "action" => action,
+        "issue" => %{
+          "id" => 1, "title" => "foo", "body" => "bar", "state" => "baz",
+          "user" => %{"id" => "bat"}
+        },
+        "repository" => %{"id" => 2}
+      }
 
-  describe "handle/2 for Issues::demilestoned" do
-    @payload %{}
+      @event build(:github_event, action: action, type: "issues")
 
-    test "is not implemented" do
-      event = build(:github_event, action: "demilestoned", type: "issues")
-      assert Issues.handle(event, @payload) == {:error, :not_fully_implemented}
+      test "is not implemented" do
+        assert Issues.handle(@event, @payload) == {:error, :not_fully_implemented}
+      end
     end
-  end
+  end)
 end