Issues webhook handling #848

begedin · 2017-07-13T14:10:55Z

Closes #828

This PR started out as a handler for the Issues "opened" webhook exclusively, but I managed to find a generalized solution for all 4 basic subtypes of this webhook.

I also believe we will be able to extend this generalised solution for other subtypes in the future.

This is still not done, but there is an uncertainty we need to discuss, so I'm creating a PR for that purpose.

What the handler does

First, the handler tries to find a GithubRepo for the issue
If no repo, {:error, :unmatched_repo} is returned, meaning the event itself will be marked as errored
If a repo is found, it then for each ProjectGithubRepo child of the GithubRepo (found by github_id, project_id), it will
- find or create a Task
- associate that task with a User (also found by github id)

What I'm unsure of is the following:

in cases other than the "opened" event, should we be creating a Task in case one was not found. Presumably, as the Project was connected to a GithubRepo, a sync was performed, so all tasks should be present, meaning this should not actually be occurring, but to code allows it, so I'm not enthusiastic about just ignoring it. Yes, we should be creating a task
if we do not find a User what should we do? This is a realistic option and something we've discussed in the past, but we never agreed on a solution fully. Do we hold a CodeCorps user locally, to assign to the Task? Do we store the user_github_id as a Task field, to be joined later? Do we create a user, which can later be merged with a newly registered/connected user? We should be creating a user

TODO

Write tests for StateManager
Write tests for Validator
Update/write tests for Handler
Write tests for ChangesetBuilder
Deal with the unmatched user case
Deal with any changes for unmatched task

joshsmith · 2017-07-13T22:36:34Z

in cases other than the "opened" event, should we be creating a Task in case one was not found? Presumably, as the Project was connected to a GithubRepo, a sync was performed, so all tasks should be present, meaning this should not actually be occurring, but to code allows it, so I'm not enthusiastic about just ignoring it.

Yes, we should be creating tasks if they're not found. Ideally, of course, we have some mechanism to prevent actually ingesting these events if the sync is still being performed. Bear in mind that a particularly large repository may take several hours of processing time. Even our own repositories are at this stage. We should probably design with this expectation in mind. Luckily we have good test cases available to us in order to get this right.

if we do not find a User what should we do? This is a realistic option and something we've discussed in the past, but we never agreed on a solution fully. Do we hold a CodeCorps user locally, to assign to the Task? Do we store the user_github_id as a Task field, to be joined later? Do we create a user, which can later be merged with a newly registered/connected user?

We create a user which can later be merged with a new user or a newly connected-to-GitHub user. There are some tasks hanging around related to this, including the one to remove the database-level reliance on a user's email being present when signing up (GitHub does not necessarily provide us with this information; more often than not, it will not).

begedin · 2017-07-17T13:12:41Z

lib/code_corps/adapter/map_transformer.ex

@@ -0,0 +1,86 @@
+defmodule CodeCorps.Adapter.MapTransformer do


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.

Have you tested this still works with Stripe locally?

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_test for the StripeConnectAccount adapter, so I added that one.

begedin · 2017-07-17T13:14:56Z

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

+    |> Changeset.validate_required([
+      :github_id, :name, :github_account_id,
+      :github_account_avatar_url, :github_account_login, :github_account_type
+    ])


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.

begedin · 2017-07-17T13:15:58Z

lib/code_corps/github/event/issues.ex

+      {:error, :repo, :unmatched_project, _steps} -> {:ok, []}
+      {:error, _errored_step, error_response, _steps} -> {:error, error_response}
+    end
+  end


Adding support for other subtypes of the Issues event, should be relatively simple from here. Most likely, it will be a matter of extending the behavior of the ChangesetBuilder module below.

begedin · 2017-07-17T13:20:11Z

lib/code_corps/github/event/issues.ex

+
+    case Repo.transaction(multi) do
+      {:ok, %{tasks: tasks}} -> {:ok, tasks}
+      {:error, :repo, :unmatched_project, _steps} -> {:ok, []}


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.

begedin · 2017-07-17T13:21:23Z

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

+    |> 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)


Likely, as we add new event subtypes for Issues, the actual changes will happen here. We may be creating TaskUser records alongside the main record, etc.

begedin · 2017-07-17T13:23:42Z

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

+    %User{}
+    |> Changeset.change(user_attrs |> UserAdapter.from_github_user())
+    |> Repo.insert
+  end


If we want to introduce the concept of something like an IdentityProvider record linked to a user record, which is something we've mentioned in the past, that would likely affect the UserLinker module here.

begedin · 2017-07-17T13:24:43Z

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

+      "id" => _, "title" => _, "body" => _, "state" => _,
+      "user" => %{"id" => _}
+    },
+    "repository" => %{"id" => _}}), do: true


This function returns true if the payload possesses the minimum required data to handle the Issues webhook.

Basically this is our "first line of defense", to make sure nothing has changed with the Github API spec and the structure of the payload is as we expect it to be.

begedin · 2017-07-17T13:26:19Z

priv/repo/migrations/20170717092127_allow_user_email_username_null.exs

+  def up do
+    alter table(:users) do
+      modify :email, :string, null: true
+      modify :username, :string, null: true


I could technically leave this one out and set the username to be the same as github_username, but I feel it just confuses things as we later merge a new/newly-connected user with this one.

begedin · 2017-07-17T14:06:59Z

@joshsmith This should be read for a review and, hopefully, merge

Final process:

We recieve an issues payload.
If the subtype is any of ~w(opened closed edited reopened) we handle it. The event will eventually be processed
If the subtype is any of ~w(assigned unassigned labeled unlabeled milestoned demilestoned) we do not handle it. The event will error out
If the subtype is any other, we do not handle it, but the return value is different from previous. Not really important right now, but might be. Wanted to make it explicit. The event will error out.
If the payload data does not contain the minimum needed to handle the event, the event will error out.

When handling

we try and find a github repo. If none is found, we error out, the event will error out
we also find or create a user
for the found github repo, if there are any ProjectGithubRepo children, for each of those we create or update a Task assigned to the found User and the associated Project
the whole process is in a transaction, so if anything goes wrong, the event errors out and none of the records are created/updated

Mapping of issue -> task

Issue state is open or closed. This becomes task status
Task state can be published or edited. Issue openedbecomespublished. closed, reopened, editedall becomeedited`
Issue body is task markdown
Task body gets generated using the same facilities we use when creating the task directly on cc

Mapping of github user --> user

id -> github_id
avatar_url -> github_avatar_url
login -> github_username

Not other mapping is performed. User is created without username or email.

joshsmith

Forgot to submit this review until way late in the evening. Everything looks generally fine. Some small comments/questions here, but nothing preventing merge if addressed.

We do need to loop back again in #849, and I'm sure I just need to see this in action to understand all the possible edge cases here, but it looks like much of the main path is figured out here.

joshsmith · 2017-07-18T04:37:30Z

lib/code_corps/adapter/map_transformer.ex

@@ -0,0 +1,86 @@
+defmodule CodeCorps.Adapter.MapTransformer do


Have you tested this still works with Stripe locally?

joshsmith · 2017-07-18T04:38:11Z

lib/code_corps/github/adapters/github_repo.ex

+    {:github_account_id, ["owner", "id"]},
+    {:github_account_avatar_url, ["owner", "avatar_url"]},
+    {:github_account_login, ["owner", "login"]},
+    {:github_account_type, ["owner", "type"]}


Just for consistency's sake, would clean up alpha ordering here (and wherever possible).

joshsmith · 2017-07-18T04:38:53Z

lib/code_corps/github/event/issues.ex

+                    {:error, :unmatched_repository}
+
+  @unimplemented_actions ~w(assigned unassigned milestoned demilestoned labeled unlabeled)
+  @implemented_actions ~w(opened closed edited reopened)


Might change ordering of these.

…ed", "reopened"

begedin added the needs clarification label Jul 13, 2017

begedin requested a review from joshsmith July 13, 2017 14:11

begedin assigned joshsmith and begedin Jul 13, 2017

begedin force-pushed the 828-issues-opened-webhook branch from 571a18e to 7021416 Compare July 17, 2017 12:05

begedin commented Jul 17, 2017

View reviewed changes

begedin force-pushed the 828-issues-opened-webhook branch 2 times, most recently from 5ed2a9a to 48f27fd Compare July 17, 2017 13:47

begedin added needs code review and removed needs clarification labels Jul 17, 2017

joshsmith reviewed Jul 18, 2017

View reviewed changes

begedin force-pushed the 828-issues-opened-webhook branch from 48f27fd to 26b10ff Compare July 18, 2017 09:32

Implemented handling of Issues webhook, for "opened", "closed", "edit…

5f48b41

…ed", "reopened"

begedin force-pushed the 828-issues-opened-webhook branch from 26b10ff to 5f48b41 Compare July 18, 2017 09:36

begedin merged commit 307eef0 into develop Jul 18, 2017

begedin deleted the 828-issues-opened-webhook branch July 18, 2017 10:04

		@@ -0,0 +1,86 @@
		defmodule CodeCorps.Adapter.MapTransformer do

Issues webhook handling #848

Issues webhook handling #848

Uh oh!

Conversation

begedin commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What the handler does

TODO

Uh oh!

joshsmith commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin commented Jul 17, 2017

Final process:

When handling

Mapping of issue -> task

Mapping of github user --> user

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

begedin commented Jul 13, 2017 •

edited

Loading