-
Notifications
You must be signed in to change notification settings - Fork 86
Issues webhook handling #848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| defmodule CodeCorps.Adapter.MapTransformer do | ||
| @moduledoc ~S""" | ||
| Module used to transform maps for the purposes of various adapters used by the | ||
| application. | ||
| """ | ||
|
|
||
| @typedoc ~S""" | ||
| A format representing how a single key should be mapped from a source. The | ||
| actual format is a 2 element tuple. | ||
| The first element is the destination key in the output map. | ||
| The second element is a list of keys representing the nested path to the key | ||
| in the source map. | ||
| For example, the tuple: | ||
| `{:target_path, ["nested", "path", "to", "source"]}` | ||
| Means that, from the source map, we need to take the nested value under | ||
| "nested" => "path" => "to" => "source" and then put it into the output map, | ||
| as a value for the key ":target_path". | ||
| """ | ||
| @type key_mapping :: {atom, list[atom]} | ||
|
|
||
| @typedoc """ | ||
| """ | ||
| @type mapping :: list(key_mapping) | ||
|
|
||
| @doc ~S""" | ||
| Takes a source map and a list of tuples representing how the source map | ||
| should be transformed into a new map, then applies the mapping | ||
| operation on each field. | ||
| """ | ||
| @spec transform(map, mapping) :: map | ||
| def transform(%{} = source_map, mapping) when is_list(mapping) do | ||
| mapping |> Enum.reduce(%{}, &map_field(&1, &2, source_map)) | ||
| end | ||
|
|
||
| @spec map_field(key_mapping, map, map) :: map | ||
| defp map_field({target_field, source_path}, %{} = target_map, %{} = source_map) do | ||
| value = get_in(source_map, source_path) | ||
| target_map |> Map.put(target_field, value) | ||
| end | ||
|
|
||
| @doc ~S""" | ||
| Performs the inverse of `&transform/2` | ||
| """ | ||
| @spec transform_inverse(map, mapping) :: map | ||
| def transform_inverse(%{} = map, mapping) when is_list(mapping) do | ||
| mapping |> Enum.reduce(%{}, &map_field_inverse(&1, &2, map)) | ||
| end | ||
|
|
||
| @spec map_field_inverse(key_mapping, map, map) :: map | ||
| defp map_field_inverse({source_field, target_path}, target_map, source_map) do | ||
| value = source_map |> Map.get(source_field) | ||
| list = target_path |> Enum.reverse | ||
| result = put_value(list, value, %{}) | ||
| deep_merge(target_map, result) | ||
| end | ||
|
|
||
| defp put_value(_, value, map) when is_nil(value), do: map | ||
| defp put_value([head | tail], value, map) do | ||
| new_value = Map.put(%{}, head, value) | ||
| put_value(tail, new_value, map) | ||
| end | ||
| defp put_value([], new_value, _map), do: new_value | ||
|
|
||
| defp deep_merge(left, right) do | ||
| Map.merge(left, right, &deep_resolve/3) | ||
| end | ||
|
|
||
| # Key exists in both maps, and both values are maps as well. | ||
| # These can be merged recursively. | ||
| defp deep_resolve(_key, left = %{}, right = %{}) do | ||
| deep_merge(left, right) | ||
| end | ||
|
|
||
| # Key exists in both maps, but at least one of the values is | ||
| # NOT a map. We fall back to standard merge behavior, preferring | ||
| # the value on the right. | ||
| defp deep_resolve(_key, _left, right) do | ||
| right | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,16 @@ | ||
| defmodule CodeCorps.GitHub.Adapters.GithubRepo do | ||
| def from_api(%{ | ||
| "id" => github_id, | ||
| "name" => name, | ||
| "owner" => %{ | ||
| "id" => github_account_id, | ||
| "avatar_url" => github_account_avatar_url, | ||
| "login" => github_account_login, | ||
| "type" => github_account_type | ||
| } | ||
| }) do | ||
| %{ | ||
| github_id: github_id, | ||
| name: name, | ||
| github_account_id: github_account_id, | ||
| github_account_avatar_url: github_account_avatar_url, | ||
| github_account_login: github_account_login, | ||
| github_account_type: github_account_type | ||
| } | ||
|
|
||
| @mapping [ | ||
| {:github_account_avatar_url, ["owner", "avatar_url"]}, | ||
| {:github_account_id, ["owner", "id"]}, | ||
| {:github_account_login, ["owner", "login"]}, | ||
| {:github_account_type, ["owner", "type"]}, | ||
| {:github_id, ["id"]}, | ||
| {:name, ["name"]} | ||
| ] | ||
|
|
||
| @spec from_api(map) :: map | ||
| def from_api(%{} = payload) do | ||
| payload |> CodeCorps.Adapter.MapTransformer.transform(@mapping) | ||
| end | ||
| def from_api(_), do: {:error, :invalid_repo_payload} | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| defmodule CodeCorps.GitHub.Adapters.Task do | ||
| @moduledoc """ | ||
| Used to adapt a GitHub issue payload into attributes for creating or updating | ||
| a `CodeCorps.Task`. | ||
| """ | ||
|
|
||
| @mapping [ | ||
| {:github_id, ["id"]}, | ||
| {:markdown, ["body"]}, | ||
| {:status, ["state"]}, | ||
| {:title, ["title"]} | ||
| ] | ||
|
|
||
| @spec from_issue(map) :: map | ||
| def from_issue(%{} = payload) do | ||
| payload |> CodeCorps.Adapter.MapTransformer.transform(@mapping) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| defmodule CodeCorps.GitHub.Adapters.User do | ||
| @moduledoc """ | ||
| Used to adapt a GitHub issue payload into attributes for creating or updating | ||
| a `CodeCorps.Task`. | ||
| """ | ||
|
|
||
| @mapping [ | ||
| {:github_avatar_url, ["avatar_url"]}, | ||
| {:github_id, ["id"]}, | ||
| {:github_username, ["login"]} | ||
| ] | ||
|
|
||
| @spec from_github_user(map) :: map | ||
| def from_github_user(%{} = payload) do | ||
| payload |> CodeCorps.Adapter.MapTransformer.transform(@mapping) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,19 +71,12 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do | |
| end | ||
|
|
||
| # transaction step 2 | ||
| @spec adapt_api_repo_list(%{api_response: any}) :: list(map) | {:error, :invalid_repo_payload} | ||
| @spec adapt_api_repo_list(%{api_response: any}) :: {:ok, list(map)} | ||
| defp adapt_api_repo_list(%{api_response: repositories}) do | ||
| adapter_results = repositories |> Enum.map(&GithubRepoAdapter.from_api/1) | ||
| case adapter_results |> Enum.all?(&valid?/1) do | ||
| true -> {:ok, adapter_results} | ||
| false -> {:error, :invalid_repo_payload} | ||
| end | ||
| {:ok, adapter_results} | ||
| end | ||
|
|
||
| @spec valid?(any) :: boolean | ||
| defp valid?({:error, _}), do: false | ||
| defp valid?(_), do: true | ||
|
|
||
| # transaction step 3 | ||
| @spec delete_repos(%{processing_installation: GithubAppInstallation.t, repo_attrs_list: list(map)}) :: aggregated_result | ||
| defp delete_repos(%{ | ||
|
|
@@ -141,18 +134,28 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do | |
| @spec create(GithubAppInstallation.t, map) :: {:ok, GithubRepo.t} | ||
| defp create(%GithubAppInstallation{} = installation, %{} = repo_attributes) do | ||
| %GithubRepo{} | ||
| |> Changeset.change(repo_attributes) | ||
| |> changeset(repo_attributes) | ||
| |> Changeset.put_assoc(:github_app_installation, installation) | ||
| |> Repo.insert() | ||
| end | ||
|
|
||
| @spec update(GithubRepo.t, map) :: {:ok, GithubRepo.t} | ||
| defp update(%GithubRepo{} = github_repo, %{} = repo_attributes) do | ||
| github_repo | ||
| |> Changeset.change(repo_attributes) | ||
| |> changeset(repo_attributes) | ||
| |> Repo.update() | ||
| end | ||
|
|
||
| @spec changeset(GithubRepo.t, map) :: Changeset.t | ||
| defp changeset(%GithubRepo{} = github_repo, %{} = repo_attributes) do | ||
| github_repo | ||
| |> Changeset.change(repo_attributes) | ||
| |> Changeset.validate_required([ | ||
| :github_id, :name, :github_account_id, | ||
| :github_account_avatar_url, :github_account_login, :github_account_type | ||
| ]) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of having the adapter implicitly doing validation of the repo payload, I switched to simply having the process (and the transaction) erroring out if there are any validation errors on any of the repos. |
||
| end | ||
|
|
||
| # transaction step 5 | ||
| @spec mark_processed(%{processing_installation: GithubAppInstallation.t}) :: {:ok, GithubAppInstallation.t} | ||
| defp mark_processed(%{processing_installation: %GithubAppInstallation{} = installation}) do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,132 @@ | ||
| defmodule CodeCorps.GitHub.Event.Issues do | ||
| @moduledoc """ | ||
| @moduledoc ~S""" | ||
| In charge of dealing with "Issues" GitHub Webhook events | ||
|
|
||
| https://developer.github.com/v3/activity/events/types/#issuesevent | ||
| """ | ||
|
|
||
| alias CodeCorps.GithubEvent | ||
| alias CodeCorps.{ | ||
| GithubEvent, | ||
| GithubRepo, | ||
| GitHub.Event.Issues.ChangesetBuilder, | ||
| GitHub.Event.Issues.Validator, | ||
| GitHub.Event.Issues.UserLinker, | ||
| ProjectGithubRepo, | ||
| Repo, | ||
| Task, | ||
| User | ||
| } | ||
| alias Ecto.{Changeset, 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) | ||
|
|
||
| @doc ~S""" | ||
| Handles the "Issues" GitHub webhook | ||
|
|
||
| @doc """ | ||
| Handles an "Issues" GitHub Webhook event | ||
| 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 | ||
|
|
||
| The general idea is | ||
| - marked the passed in event as "processing" | ||
| - do the work | ||
| - marked the passed in event as "processed" or "errored" | ||
| 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 | ||
|
|
||
| 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. | ||
| """ | ||
| def handle(%GithubEvent{}, %{}), do: {:error, :not_fully_implemented} | ||
| @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 | ||
| 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 _ -> find_repo(payload) end) | ||
| |> Multi.run(:user, fn _ -> UserLinker.find_or_create_user(payload) end) | ||
| |> Multi.run(:tasks, &sync_all(&1, payload)) | ||
|
|
||
| case Repo.transaction(multi) do | ||
| {:ok, %{tasks: tasks}} -> {:ok, tasks} | ||
| {:error, :repo, :unmatched_project, _steps} -> {:ok, []} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want the transaction to fail if there are no tasks to create/update due to unmatched projects. If it does not fail, we will end up creating a user for no reason. However, this transaction failure should not be interpreted as a failure to handle the webhook, so we translate it into a success with no data updated. |
||
| {:error, _errored_step, error_response, _steps} -> {:error, error_response} | ||
| end | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding support for other subtypes of the |
||
|
|
||
| @spec find_repo(map) :: {:ok, GithubRepo.t} | {:error, :unmatched_repository} | {:error, :unmatched_project} | ||
| defp find_repo(%{"repository" => %{"id" => github_id}}) do | ||
| case GithubRepo |> Repo.get_by(github_id: github_id) |> Repo.preload(:project_github_repos) do | ||
| # a GithubRepo with at least some ProjectGithubRepo children | ||
| %GithubRepo{project_github_repos: [_ | _]} = github_repo -> {:ok, github_repo} | ||
| # a GithubRepo with no ProjectGithubRepo children | ||
| %GithubRepo{project_github_repos: []} -> {:error, :unmatched_project} | ||
| nil -> {:error, :unmatched_repository} | ||
| end | ||
| end | ||
|
|
||
| @spec sync_all(map, map) :: {:ok, list(Task.t)} | ||
| defp sync_all( | ||
| %{ | ||
| repo: %GithubRepo{project_github_repos: project_github_repos}, | ||
| user: %User{} = user | ||
| }, | ||
| %{} = payload) do | ||
|
|
||
| project_github_repos | ||
| |> Enum.map(&sync(&1, user, payload)) | ||
| |> aggregate() | ||
| end | ||
|
|
||
| @spec sync(ProjectGithubRepo.t, User.t, map) :: {:ok, ProjectGithubRepo.t} | {:error, Changeset.t} | ||
| defp sync(%ProjectGithubRepo{} = project_github_repo, %User{} = user, %{} = payload) do | ||
| project_github_repo | ||
| |> find_or_init_task(payload) | ||
| |> ChangesetBuilder.build_changeset(payload, project_github_repo, user) | ||
| |> commit() | ||
| end | ||
|
|
||
| @spec find_or_init_task(ProjectGithubRepo.t, map) :: Task.t | ||
| defp find_or_init_task(%ProjectGithubRepo{project_id: project_id}, %{"issue" => %{"id" => github_id}}) do | ||
| case Task |> Repo.get_by(github_id: github_id, project_id: project_id) do | ||
| nil -> %Task{} | ||
| %Task{} = task -> task | ||
| end | ||
| end | ||
|
|
||
| @spec commit(Changeset.t) :: {:ok, Task.t} | {:error, Changeset.t} | ||
| defp commit(%Changeset{data: %Task{id: nil}} = changeset), do: changeset |> Repo.insert | ||
| defp commit(%Changeset{} = changeset), do: changeset |> Repo.update | ||
|
|
||
| @spec aggregate(list({:ok, Task.t})) :: {:ok, list(Task.t)} | ||
| defp aggregate(results) do | ||
| results | ||
| |> Enum.map(&Tuple.to_list/1) | ||
| |> Enum.map(&List.last/1) | ||
| |> (fn tasks -> {:ok, tasks} end).() | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| defmodule CodeCorps.GitHub.Event.Issues.ChangesetBuilder do | ||
| @moduledoc ~S""" | ||
| In charge of building a `Changeset` to update a `Task` with, when handling an | ||
| Issues webhook. | ||
| """ | ||
|
|
||
| alias CodeCorps.{ | ||
| GitHub.Event.Issues.StateMapper, | ||
| Services.MarkdownRendererService, | ||
| ProjectGithubRepo, | ||
| Task, | ||
| User | ||
| } | ||
| alias CodeCorps.GitHub.Adapters.Task, as: TaskAdapter | ||
| alias Ecto.Changeset | ||
|
|
||
| @doc ~S""" | ||
| Constructs a changeset for syncing a task when processing an Issues webhook | ||
| """ | ||
| @spec build_changeset(Task.t, map, ProjectGithubRepo.t, User.t) :: Changeset.t | ||
| def build_changeset( | ||
| %Task{} = task, | ||
| %{"issue" => issue_attrs} = payload, | ||
| %ProjectGithubRepo{project_id: project_id}, | ||
| %User{id: user_id}) do | ||
|
|
||
| task | ||
| |> Changeset.change(issue_attrs |> TaskAdapter.from_issue()) | ||
| |> Changeset.put_change(:state, payload |> StateMapper.get_state()) | ||
| |> MarkdownRendererService.render_markdown_to_html(:markdown, :body) | ||
| |> Changeset.put_change(:project_id, project_id) | ||
| |> Changeset.put_change(:user_id, user_id) | ||
| |> Changeset.validate_required([:project_id, :user_id, :markdown, :body, :title]) | ||
| |> Changeset.assoc_constraint(:project) | ||
| |> Changeset.assoc_constraint(:user) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely, as we add new event subtypes for |
||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were using this module as a Stripe util module, but the approach it uses is generic and can be reused for the purposes of GitHub adapters, so I moved it to outside of Stripe's scope.
I also did some renaming and added documentation to make it clearer what the functions in it actually do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this still works with Stripe locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing, but generally, the stripe adapter layer is already well-tested.
I went through it one more time and the only thing that was missing was a
from_params_testfor theStripeConnectAccountadapter, so I added that one.