Merge pull request #1048 from code-corps/1045-standardize-issue-comment-webhook

Standardized output for CodeCorps.GitHub.Event.IssueComment

Author: joshsmith

lib/code_corps/github/event/issue_comment.ex

@@ -1,10 +1,11 @@
 defmodule CodeCorps.GitHub.Event.IssueComment do
   @moduledoc ~S"""
-  In charge of dealing with "IssueComment" GitHub Webhook events
-
-  https://developer.github.com/v3/activity/events/types/#issuecommentevent
+  In charge of handling a GitHub Webhook payload for the IssueComment event type
+  [https://developer.github.com/v3/activity/events/types/#issuecommentevent](https://developer.github.com/v3/activity/events/types/#issuecommentevent)
   """
 
+  @behaviour CodeCorps.GitHub.Event.Handler
+
   alias CodeCorps.{
     Comment,
     GithubEvent,
@@ -18,63 +19,86 @@ defmodule CodeCorps.GitHub.Event.IssueComment do
   }
   alias Ecto.Multi
 
-  @typep outcome :: {:ok, list(Comment.t)} |
-                    {:error, :unexpected_payload} |
-                    {:error, :unexpected_action} |
-                    {:error, :unmatched_repository}
+  @type outcome :: {:ok, list(Comment.t)} |
+                   {:error, :unexpected_action} |
+                   {:error, :unexpected_payload} |
+                   {:error, :repository_not_found} |
+                   {:error, :validation_error_on_inserting_user_for_task} |
+                   {:error, :multiple_github_users_matched_same_cc_user_for_task} |
+                   {:error, :validation_error_on_inserting_user_for_comment} |
+                   {:error, :multiple_github_users_matched_same_cc_user_for_comment} |
+                   {:error, :validation_error_on_syncing_tasks} |
+                   {:error, :validation_error_on_syncing_comments} |
+                   {:error, :unexpected_transaction_outcome}
 
   @doc ~S"""
-  Handles the "Issues" GitHub webhook
+  Handles the "IssueComment" 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 `Task`
-      - if initializing, associate with existing, or create `User`
-    - find or initialize a `Comment`
-      - if initializing, associate with existing, or create `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 issue part of the payload with a `CodeCorps.User` using `CodeCorps.GitHub.Event.Issues.UserLinker`
+  - match comment part of the payload with a `CodeCorps.User` using `CodeCorps.GitHub.Event.IssueComment.UserLinker`
+  - for each `CodeCorps.ProjectGithubRepo` belonging to matched repo
+    - match and update, or create a `CodeCorps.Task` on the associated `CodeCorps.Project`
+    - match and update, or create a `CodeCorps.Comment` associated to `CodeCorps.Task`
 
-  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) comments.
 
-  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 ~w(created edited deleted) do
-    case payload |> IssueComment.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.append(payload |> operational_multi())
+    |> Repo.transaction
+    |> marshall_result()
+  end
+
+  @spec operational_multi(map) :: Multi.t
+  defp operational_multi(%{"action" => action} = payload) when action in ~w(created edited) do
+    Multi.new
+    |> Multi.run(:repo, fn _ -> RepoFinder.find_repo(payload) end)
+    |> Multi.run(:issue_user, fn _ -> Issues.UserLinker.find_or_create_user(payload) end)
+    |> Multi.run(:comment_user, fn _ -> IssueComment.UserLinker.find_or_create_user(payload) end)
+    |> Multi.run(:tasks, fn %{repo: github_repo, issue_user: user} -> TaskSyncer.sync_all(github_repo, user, payload) end)
+    |> Multi.run(:comments, fn %{tasks: tasks, comment_user: user} -> CommentSyncer.sync_all(tasks, user, payload) end)
+  end
+  defp operational_multi(%{"action" => "deleted"} = payload) do
+    Multi.new
+    |> Multi.run(:comments, fn _ -> CommentDeleter.delete_all(payload) end)
   end
-  def handle(%GithubEvent{action: _action}, _payload), do: {:error, :unexpected_action}
+  defp operational_multi(%{}), do: Multi.new
 
-  @spec do_handle(map) :: {:ok, list(Comment.t)} | {:error, :unmatched_repository}
-  defp do_handle(%{"action" => action} = payload) when action in ~w(created edited) do
-    multi =
-      Multi.new
-      |> Multi.run(:repo, fn _ -> RepoFinder.find_repo(payload) end)
-      |> Multi.run(:issue_user, fn _ -> Issues.UserLinker.find_or_create_user(payload) end)
-      |> Multi.run(:comment_user, fn _ -> IssueComment.UserLinker.find_or_create_user(payload) end)
-      |> Multi.run(:tasks, fn %{repo: github_repo, issue_user: user} -> TaskSyncer.sync_all(github_repo, user, payload) end)
-      |> Multi.run(:comments, fn %{tasks: tasks, comment_user: user} -> CommentSyncer.sync_all(tasks, user, payload) end)
+  @spec marshall_result(tuple) :: tuple
+  defp marshall_result({:ok, %{comments: comments}}), do: {:ok, comments}
+  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, :repo, :unmatched_project, _steps}), do: {:ok, []}
+  defp marshall_result({:error, :repo, :unmatched_repository, _steps}), do: {:error, :repository_not_found}
+  defp marshall_result({:error, :issue_user, %Ecto.Changeset{}, _steps}), do: {:error, :validation_error_on_inserting_user_for_task}
+  defp marshall_result({:error, :issue_user, :multiple_users, _steps}), do: {:error, :multiple_github_users_matched_same_cc_user_for_task}
+  defp marshall_result({:error, :comment_user, %Ecto.Changeset{}, _steps}), do: {:error, :validation_error_on_inserting_user_for_comment}
+  defp marshall_result({:error, :comment_user, :multiple_users, _steps}), do: {:error, :multiple_github_users_matched_same_cc_user_for_comment}
+  defp marshall_result({:error, :tasks, {_tasks, _errors}, _steps}), do: {:error, :validation_error_on_syncing_tasks}
+  defp marshall_result({:error, :comments, {_comments, _errors}, _steps}), do: {:error, :validation_error_on_syncing_comments}
+  defp marshall_result({:error, _errored_step, _error_response, _steps}), do: {:error, :unexpected_transaction_outcome}
 
-    case Repo.transaction(multi) do
-      {:ok, %{comments: comments}} -> {:ok, comments}
-      {: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 |> IssueComment.Validator.valid? do
+      true -> {:ok, :valid}
+      false -> {:error, :invalid}
     end
   end
-  defp do_handle(%{"action" => "deleted"} = payload) do
-    CommentDeleter.delete_all(payload)
-  end
+
+  @implemented_actions ~w(created edited deleted)
+  @spec validate_action(map) :: {:ok, :implemented} | {:error, :unexpected_action}
+  defp validate_action(%{"action" => action}) when action in @implemented_actions, do: {:ok, :implemented}
+  defp validate_action(%{}), do: {:error, :unexpected_action}
 end

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

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

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

@@ -15,7 +15,7 @@ defmodule CodeCorps.GitHub.Event.IssueCommentTest do
   }
 
   describe "handle/2" do
-    @payload load_event_fixture("issue_comment_created")
+    @payload load_event_fixture("issue_comment_created") |> Map.put("action", "foo")
 
     test "returns error if action of the event is wrong" do
       event = build(:github_event, action: "foo", type: "issue_comment")
@@ -96,7 +96,7 @@ defmodule CodeCorps.GitHub.Event.IssueCommentTest do
       end
 
       test "with unmatched both users, returns error if unmatched repository" do
-        assert IssueComment.handle(@event, @payload) == {:error, :unmatched_repository}
+        assert IssueComment.handle(@event, @payload) == {:error, :repository_not_found}
         refute Repo.one(User)
       end
 
@@ -180,7 +180,7 @@ defmodule CodeCorps.GitHub.Event.IssueCommentTest do
 
         _issue_user = insert(:user, github_id: issue_user_github_id)
 
-        assert IssueComment.handle(@event, @payload) == {:error, :unmatched_repository}
+        assert IssueComment.handle(@event, @payload) == {:error, :repository_not_found}
       end
 
       test "with unmatched issue user, matched comment_user, passes with no changes made if no matching projects" do
@@ -255,7 +255,7 @@ defmodule CodeCorps.GitHub.Event.IssueCommentTest do
 
         _comment_user = insert(:user, github_id: comment_user_github_id)
 
-        assert IssueComment.handle(@event, @payload) == {:error, :unmatched_repository}
+        assert IssueComment.handle(@event, @payload) == {:error, :repository_not_found}
       end
 
       test "with matched issue and comment_user, passes with no changes made if no matching projects" do
@@ -350,7 +350,7 @@ defmodule CodeCorps.GitHub.Event.IssueCommentTest do
         insert(:user, github_id: comment_user_github_id)
         insert(:user, github_id: issue_user_github_id)
 
-        assert IssueComment.handle(@event, @payload) == {:error, :unmatched_repository}
+        assert IssueComment.handle(@event, @payload) == {:error, :repository_not_found}
       end
 
       test "returns error if payload is wrong" do