Merge pull request #1185 from code-corps/fix-partially-supported-github-errored-events

Cleaned up unsupported and ignored event handling

Author: joshsmith

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

@@ -16,15 +16,13 @@ defmodule CodeCorps.GitHub.Event.Installation do
   alias Ecto.{Changeset, Multi}
 
   @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}
+                   {: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}
 
   @doc """
   Handles the "Installation" GitHub Webhook event.
@@ -51,7 +49,6 @@ defmodule CodeCorps.GitHub.Event.Installation do
   def handle(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)
@@ -84,8 +81,6 @@ defmodule CodeCorps.GitHub.Event.Installation do
   @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}
@@ -103,11 +98,4 @@ defmodule CodeCorps.GitHub.Event.Installation do
       false -> {:error, :invalid}
     end
   end
-
-  @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_repositories/installation_repositories.ex

@@ -20,11 +20,10 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
   alias Ecto.{Changeset, Multi}
 
   @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}
+                   {:error, :unmatched_installation} |
+                   {:error, :unexpected_payload} |
+                   {:error, :validation_error_on_syncing_repos} |
+                   {:error, :unexpected_transaction_outcome}
 
   @doc """
   Handles an "InstallationRepositories" GitHub Webhook event. The event could be
@@ -47,7 +46,6 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
   def handle(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
@@ -62,11 +60,6 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
     end
   end
 
-  @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}
-
   @spec match_installation(map) :: {:ok, GithubAppInstallation.t} |
                                    {:error, :unmatched_installation}
   defp match_installation(%{"installation" => %{"id" => github_id}}) do
@@ -123,7 +116,6 @@ defmodule CodeCorps.GitHub.Event.InstallationRepositories do
   @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}

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

@@ -12,9 +12,7 @@ defmodule CodeCorps.GitHub.Event.IssueComment do
   }
   alias GitHub.Sync
 
-  @type outcome :: Sync.outcome
-                 | {:error, :unexpected_action}
-                 | {:error, :unexpected_payload}
+  @type outcome :: Sync.outcome | {:error, :unexpected_payload}
 
   @doc ~S"""
   Handles the "IssueComment" GitHub webhook
@@ -27,26 +25,18 @@ defmodule CodeCorps.GitHub.Event.IssueComment do
   """
   @spec handle(map) :: outcome
   def handle(payload) do
-    with {:ok, :valid} <- validate_payload(payload),
-         {:ok, :implemented} <- validate_action(payload) do
+    with {:ok, :valid} <- validate_payload(payload) do
       Sync.issue_comment_event(payload)
     else
       {:error, error} -> {:error, error}
     end
   end
 
-  @spec validate_payload(map) :: {:ok, :valid}
-                               | {:error, :unexpected_payload}
+  @spec validate_payload(map) :: {:ok, :valid} | {:error, :unexpected_payload}
   defp validate_payload(%{} = payload) do
     case payload |> Validator.valid? do
       true -> {:ok, :valid}
       false -> {:error, :unexpected_payload}
     end
   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/issues/issues.ex

@@ -13,10 +13,7 @@ defmodule CodeCorps.GitHub.Event.Issues do
   }
   alias GitHub.Sync
 
-  @type outcome :: Sync.outcome
-                 | {:error, :unexpected_action}
-                 | {:error, :not_fully_implemented}
-                 | {:error, :unexpected_payload}
+  @type outcome :: Sync.outcome | {:ok, :ignored} | {:error, :unexpected_payload}
 
   @doc ~S"""
   Handles the "Issues" GitHub webhook
@@ -29,8 +26,7 @@ defmodule CodeCorps.GitHub.Event.Issues do
   """
   @spec handle(map) :: outcome
   def handle(payload) do
-    with {:ok, :valid} <- validate_payload(payload),
-         {:ok, :implemented} <- validate_action(payload) do
+    with {:ok, :valid} <- validate_payload(payload) do
       Sync.issue_event(payload)
     else
       {:error, error} -> {:error, error}
@@ -45,14 +41,4 @@ defmodule CodeCorps.GitHub.Event.Issues do
       false -> {:error, :unexpected_payload}
     end
   end
-
-  @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 }
-                              | {:error, :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}
 end

lib/code_corps/github/event/pull_request/pull_request.ex

@@ -13,10 +13,7 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
   }
   alias GitHub.Sync
 
-  @type outcome :: Sync.outcome
-                 | {:error, :unexpected_action}
-                 | {:error, :not_fully_implemented}
-                 | {:error, :unexpected_payload}
+  @type outcome :: Sync.outcome | {:error, :unexpected_payload}
 
   @doc ~S"""
   Handles the "PullRequest" GitHub webhook
@@ -29,8 +26,7 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
   """
   @spec handle(map) :: outcome
   def handle(payload) do
-    with {:ok, :valid} <- validate_payload(payload),
-         {:ok, :implemented} <- validate_action(payload) do
+    with {:ok, :valid} <- validate_payload(payload) do
       Sync.pull_request_event(payload)
     else
       {:error, error} -> {:error, error}
@@ -45,14 +41,4 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
       false -> {:error, :unexpected_payload}
     end
   end
-
-  @implemented_actions ~w(opened closed edited reopened)
-  @unimplemented_actions ~w(assigned unassigned review_requested review_request_removed labeled unlabeled)
-
-  @spec validate_action(map) :: {:ok, :implemented}
-                              | {:error, :not_fully_implemented }
-                              | {:error, :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}
 end

lib/code_corps/github/webhook/event_support.ex

@@ -3,23 +3,66 @@ defmodule CodeCorps.GitHub.Webhook.EventSupport do
   Determines event support for a GitHub event type
   """
 
-  @supported_events ~w(
-    installation installation_repositories issue_comment issues pull_request
-  )
+  @type support_status :: :supported | :unsupported | :ignored
 
-  @type support_status :: :supported | :unsupported
+  @supported_events [
+    {"installation", "created"},
+    {"installation_repositories", "added"},
+    {"installation_repositories", "removed"},
+    {"issue_comment", "created"},
+    {"issue_comment", "edited"},
+    {"issue_comment", "deleted"},
+    {"issues", "opened"},
+    {"issues", "edited"},
+    {"issues", "closed"},
+    {"issues", "reopened"},
+    {"pull_request", "opened"},
+    {"pull_request", "edited"},
+    {"pull_request", "closed"},
+    {"pull_request", "reopened"},
+  ]
 
-  @doc """
-  Returns :supported if the GitHub event type is in the list of events we
-  support, :unsupported otherwise.
-  """
-  @spec status(any) :: support_status
-  def status(event_type) when event_type in @supported_events, do: :supported
-  def status(_), do: :unsupported
+  @doc ~S"""
+  Utility function. Returns list of supported events as `{type, action}` tuples.
 
-  @doc """
-  Convenience function. Makes the internal list of supported events public.
+  Supported events are events of types and actions we currently fully support.
   """
-  @spec supported_events :: list
+  @spec supported_events :: list(tuple)
   def supported_events, do: @supported_events
+
+  @unsupported_events [
+    {"installation", "deleted"},
+    {"issues", "assigned"},
+    {"issues", "unassigned"},
+    {"issues", "labeled"},
+    {"issues", "unlabeled"},
+    {"issues", "milestoned"},
+    {"issues", "demilestoned"},
+    {"pull_request", "assigned"},
+    {"pull_request", "unassigned"},
+    {"pull_request", "review_requested"},
+    {"pull_request", "review_request_removed"},
+    {"pull_request", "labeled"},
+    {"pull_request", "unlabeled"},
+    {"pull_request", "synchronize"},
+  ]
+
+  @doc ~S"""
+  Utility function. Returns list of unsupported events as `{type, action}`
+  tuples.
+
+  Unsupported events are events of types we technically support, but actions we
+  do not yet implement the handling of.
+  """
+  @spec unsupported_events :: list(tuple)
+  def unsupported_events, do: @unsupported_events
+
+  @doc ~S"""
+  Returns `:handled` if the GitHub event/action is being handled by the system,
+  `:ignored` otherwise.
+  """
+  @spec status(String.t, String.t) :: support_status
+  def status(type, action) when {type, action} in @supported_events, do: :supported
+  def status(type, action) when {type, action} in @unsupported_events, do: :unsupported
+  def status(_type, _action), do: :ignored
 end

lib/code_corps/github/webhook/handler.ex

@@ -11,46 +11,59 @@ defmodule CodeCorps.GitHub.Webhook.Handler do
     GitHub.Event.IssueComment,
     GitHub.Event.Issues,
     GitHub.Event.PullRequest,
-    GitHub.Webhook.EventSupport,
     Repo
   }
 
   @doc """
-  Handles a GitHub event based on its type.
+  Handles a fully supported GitHub event based on its type and action.
+
+  The handling process consistes of 3 steps
+
+  - create event record marked as "unprocessed"
+  - mark event record as processing and handle it
+  - mark event record as processed or errored depending on handling outcome
   """
-  def handle(type, id, payload) do
-    with %{} = params <- build_params(type, id, payload),
-         {:ok, %GithubEvent{} = event} <- params |> create_event(),
-         {:ok, %GithubEvent{status: "processing"} = event} <- event |> Event.start_processing
-    do
-      payload |> do_handle(type) |> Event.stop_processing(event)
+  @spec handle_supported(String.t, String.t, map) :: {:ok, GithubEvent.t}
+  def handle_supported(type, id, %{} = payload) do
+    with {:ok, %GithubEvent{} = event} <- type |> build_params(id, "unprocessed", payload) |> create_event() do
+      payload |> apply_handler(type) |> Event.stop_processing(event)
     end
   end
 
-  defp build_params(type, id, %{"action" => action, "sender" => _} = payload) do
+  @doc ~S"""
+  Handles an unsupported supported GitHub event.
+
+  "unsupported" means that, while we generally support this event type,
+  we do not yet support this specific event action.
+
+  The process consistes of simply storing the event and marking it as
+  "unsupported".
+  """
+  @spec handle_unsupported(String.t, String.t, map) :: {:ok, GithubEvent.t}
+  def handle_unsupported(type, id, %{} = payload) do
+    type |> build_params(id, "unsupported", payload) |> create_event()
+  end
+
+  @spec build_params(String.t, String.t, String.t, map) :: map
+  defp build_params(type, id, status, %{"action" => action} = payload) do
     %{
       action: action,
       github_delivery_id: id,
       payload: payload,
-      status: type |> get_status(),
+      status: status,
       type: type
     }
   end
 
-  defp create_event(params) do
-    %GithubEvent{} |> GithubEvent.changeset(params) |> Repo.insert
-  end
-
-  defp get_status(type) do
-    case EventSupport.status(type) do
-      :unsupported -> "unhandled"
-      :supported -> "unprocessed"
-    end
+  @spec create_event(map) :: {:ok, GithubEvent.t}
+  defp create_event(%{} = params) do
+    %GithubEvent{} |> GithubEvent.changeset(params) |> Repo.insert()
   end
 
-  defp do_handle(payload, "installation"), do: Installation.handle(payload)
-  defp do_handle(payload, "installation_repositories"), do: InstallationRepositories.handle(payload)
-  defp do_handle(payload, "issue_comment"), do: IssueComment.handle(payload)
-  defp do_handle(payload, "issues"), do: Issues.handle(payload)
-  defp do_handle(payload, "pull_request"), do: PullRequest.handle(payload)
+  @spec apply_handler(map, String.t) :: tuple
+  defp apply_handler(payload, "installation"), do: Installation.handle(payload)
+  defp apply_handler(payload, "installation_repositories"), do: InstallationRepositories.handle(payload)
+  defp apply_handler(payload, "issue_comment"), do: IssueComment.handle(payload)
+  defp apply_handler(payload, "issues"), do: Issues.handle(payload)
+  defp apply_handler(payload, "pull_request"), do: PullRequest.handle(payload)
 end