Integration of Tasks with GitHub issues #805

npendery · 2017-05-07T22:52:02Z

Closes #803, #804, #806

What's in this PR?

Add Github service create_issue\3 function
Add testing to task create function to account for github creation
Add tentacat dependency to connect to Github
Update project to include github_owner and github_repo fields

To do

Add default user info for github issue creation

References

Fixes #804

begedin · 2017-05-08T07:55:39Z

lib/code_corps/github.ex

+      title: attributes[:title],
+      body: attributes[:body]
+    }
+  end


This seems sort of redundant, but my guess is, the intention was to have an explicit adapter function?

Pulled it out because I was thinking of reusing it for the future update_issue function. Also was thinking we might expand it to be more fields (when we want to be able to assign users, labels, etc.)

Never mind, we can come back and use this once we add the functionality for that, will update

begedin · 2017-05-08T08:03:13Z

web/controllers/task_controller.ex

+      current_user = Guardian.Plug.current_resource(conn)
+      github_id = github_module().create_issue(attributes, project, current_user)
+      attributes = Map.merge(attributes, %{"github_id" => github_id})
+    end


I apologise if I'm reviewing too early and it was already your intention to do this, but this behavior would be what we'd expect to put into the task_service module described in #804

Hey @begedin, I initially did that and was wondering the reasoning for pulling this out into a service? There really isnt a lot of complicated logic (most of that happens in the Github service) and wondering why we would create a task service just to copy the lines we have here? Are we thinking of using it beyond the CRUD actions? thanks!

There are no strong reasons right now, but

It's more in line with how Phoenix 1.3 will structure modules.

A clean, non-controller module will be easier to test for various outcomes.

The code will very likely grow beyond these few lines.

Generally, we like to keep our controller code as tiny as possible.

Cool, thanks for the points! Will make the change

begedin · 2017-05-08T08:04:03Z

web/controllers/task_controller.ex

+
+  defp github_module do
+    Application.get_env(:code_corps, :github)
+  end


I'm thinking maybe we call this github_client or even just github

npendery · 2017-05-16T22:01:24Z

@begedin Mind looking over this for me? Thanks!

begedin · 2017-05-17T08:54:59Z

lib/code_corps/github.ex

+      Map.take(task, [:title, :body]),
+      client
+    )
+    case request do


Maybe call this a result or a request_result. Not a big deal, though.

begedin · 2017-05-17T09:08:46Z

lib/code_corps/github_testing.ex

+    task
+    |> Task.github_changeset(%{"github_id" => 1})
+    |> Repo.update()
+  end


Great work on this one.

begedin · 2017-05-17T09:11:38Z

priv/repo/migrations/20170505224222_add_github_repo_and_owner_to_project.exs

+  def change do
+    alter table(:projects) do
+      add :github_repo, :string
+      add :github_owner, :string


I think this means we will no longer require a github_id. Same would go with ember's side. Would you agree?

Yeah, reviewing the Github docs more it looks like we can use the repo name for everything we need

begedin · 2017-05-17T09:13:41Z

@npendery This is great work and something that's going to get merged ASAP, but sadly, not before we're done with the connect feature.

joshsmith · 2017-05-25T21:36:37Z

@npendery we're working to get the GitHub connect stuff merged now and then can focus on this again shortly.

begedin · 2017-07-20T11:45:15Z

Hey @npendery

We're finally at a point with the rest of the project where this can be continued and hopefully wrapped up. If you're still interested in doing so, please let me know and feel free to start by resolving conflicts, etc.

Also note that, with the new knowledge, I've updated the issue a good amount, so feel free to go back to it for the update.

For converting task attributes into github issue attributes, you'll probably want to extend the GitHub.Adapters.Task or even add a GitHub.Adapters.Issue

Finally, if you're able to only get parts of this done, feel free to do so. If it's a self contained part of the solution, we will be more then happy to merge so other parts can be done separately.

Feel free to also submit separate smaller PRs, for parts of the solution, such as the adapter.

begedin · 2017-08-09T14:33:36Z

@npendery I've rebased this, but it looks like most of the changes here are also contained in #806 and it may be easier to just continue from there. I've added some much overdue information into that one.

Let me know how you feel about continuing this, but my guess is, either way, we will be closing this one and focusing on #806

begedin · 2017-08-14T13:52:04Z

Started moving this towards the new approach.

TODOS:

New migration is wrong. github_id should be github_issue_number, not github_issue_id
We should use tentacat, but with using it, and in other ways to, relying on bypass across the app is problematic. I'm considering other options based on stuff I've learned on a client project. Will write up a suggestion as soon as I can
Some tests are broken due to the column rename. I've also written some placeholder tests

joshsmith · 2017-08-17T05:45:40Z

I'm considering other options based on stuff I've learned on a client project. Will write up a suggestion as soon as I can

@begedin I'm curious what you mean here.

begedin · 2017-08-17T06:39:26Z

In our client project, I've found a relatively simple way to do mocks of the whole api layer.

Basically, there is a think api module which wraps external requests. This module is then mocked in tests that need it using a macro. What that ends up looking like is something along the lines of

with_mock_api(MockModule) do
  do_stuff
end

In that project, we have a basic success mock, where all requests return the default successful response, a basic failure module, where all requests return the default failure response, and with the way the macro works, we can define simple modules with specific responses, "inline".

In our specific case, we would probably mock the request module. The important part is, we need to make sure all our external requests are centralized, which is already something we strive to do.

begedin · 2017-08-29T15:59:03Z

@joshsmith I've been having trouble finding a nice way to implement things here.

The way I see it, we have two issues

we can't really mock Tentacat using bypass
it's difficult to keep our mocks in check with things scatted a bit across CodeCorps.GitHub, CodeCorps.GitHub.Request, etc.

My latest commit is just a bunch of scattered things that I'm not sure should be kept in, but I wanted to at least show some of it, so I pushed it up. Feel free to remove if you don't find it useful.

I would like to direct your attention to the with_mock_api macro which I wrote. It allows us to define a tiny module which could handle a custom request by asserting, sending a custom response, etc and wrap a code calling our api forcing it to call that module instead.

In order for this to work, we cannot load our api module from the config using a module @variable. Instead, it needs to be a function, so it's evaluated at execution time, not compile time.

I've defined it due to the way we use a similar approach with SparkPost in our client project.
We could, in addition to this, have a with_mock_tentacat macro as well.

These could be useful tools depending on how we continue. Really, though, the issue here is
that our communication layer for github is a bit too scattered to be easily testable. I'm not even 100% positive using tentacat here is worth it, or even possible (I'm not sure how it works with integrations).

I'm sorry I'm just listing problems instead of offering solutions, but I'm having difficulties wrapping my head around this yet.

Considerations

We need to figure out if we can use tentacat here
We need to decide if it's worth using tentacat here, considering the scatteredness it creates
We need to consolidate our own github requests to make them more easily mockable
We need to consider ditching bypass in favor of the mock approach. It may affect how we consider other things listed here.

Approaches for mocking

We have a module with a request method. All requests go through that method. We mock that module, assert it was called with proper arguments.
We have a module with a bunch of "flat" methods such as create_issue, list_repositories, etc, and we mock those.

If we use tentacat, approach 2 is still compatible. For example, create_issue would internally call Tentacat.Issues.create, etc. Approach 1 is not compatible directly, but we could work around it.

I'm leaning towards 2 if we decide to stick with tentacat and 1 if we decide to drop it.

begedin · 2017-09-06T11:05:03Z

Also note that we can close #806 as well, when this is merged.

joshsmith · 2017-09-07T16:35:42Z

@begedin it appears there's a merge conflict now from the recent merge in the policy.

begedin · 2017-09-08T05:33:16Z

@joshsmith Rebased and resolved

joshsmith · 2017-09-14T22:07:32Z

lib/code_corps_web/plugs/ids_to_integers.ex

+  defp convert?("user_role_id"), do: true
+  defp convert?("user_skill_id"), do: true
+  defp convert?("user_task_id"), do: true
+  defp convert?(_other), do: false


Would it not make more sense to here to check if it's in a list? i.e. like how Ecto validations work?

joshsmith · 2017-09-15T00:08:04Z

I just pushed up some fixes that I identified in a new commit.

Possibly outside the scope of this PR, but some other things I've noticed while testing:

When users are created on CC because we don't have any matching info from GH:
- They don't receive a username
- They don't receive a default_color
- They don't have an avatar (it's a broken image), although we do have a GitHub avatar they could use
- Their sign up context is default; this should probably be set to github or github-api instead
A user gets created for code-corps-local[bot] when an issue is opened. This likely shouldn't be happening, but I'm not quite sure how we should approach this situation.
I got an opened event for issues (created by a GH user that had no corresponding user on CC) that resulted in no issue being created, a 200 returned, a processed status, and no user created on CC. That means the flow is broken and it's not clear where it's breaking down.
It's really hard to debug what's happening when something goes wrong since we're not logging anything. I did end up logging at least the 401 returned by GitHub, which helped me identify that our timestamp code was breaking. But it took quite a long amount of time to get down the stack.

We can break these out into some follow-up issues if necessary since I'm not positive any of this relates to the PR at hand (aside from the bot situation above). I was ultimately able to create issues effectively (see https://github.com/code-corps/test/issues). 7-11 are tests created from my localhost, and 12 is the test created on GH that then failed to sync properly to my localhost.

begedin · 2017-09-15T08:50:02Z

Created When creating a user from github, merge github info into user info #938 for handling github user info when creating user.
Created Handling bot user when handling github webhook #939 for handling bot user when a non-connected user creates a task on cc
Created TaskSyncer.sync_all can return error changesets that get swallowed by our GitHub issues event handler without error #940 to verify what goes wrong that would cause user not to be created when issue is opened on github directly

…refactors - if task params containg a "github_repo_id", an issue is created on github alongside task and task is associated with issue - refactors: - removed reliance on ja_resource and canary from task controller - extracted task creation into separate module - extracted task-related queries into separate module - removed some unused code - moved some code around to follow boundex context convention - fixed several dialyzer warnings

…sk query

…expiration

begedin · 2017-09-15T09:27:12Z

@joshsmith I rebased and resolved conflicts again.

I think we need the longs, but I also think we should mute them in the test environment, since they pollute the output.

config :logger, level: :warn changing this to :error may even be enough. It would also mute the clouded warning, but really, that one should also be rewritten so an env variable is not needed in tests, and either way, the tests, if they get broken, will fail on CI.

Some notes on rebasing if there's a need for you to do so:

api.ex and mix.exs are easy
for mix.lock, simply take the entire HEAD version, and then remove the tentacat entry from it.
you'll have to mix deps.get one package at a time. If you just get all the packages, you'll pull the latest stripity_stripe, since we target the 2.0 branch, which will break things.

joshsmith · 2017-09-15T16:39:20Z

@begedin should we place these rebasing notes in the README somewhere? Seems like it might end up being a fairly common thing to accidentally pull in these changes from the 2.0 branch since we're not locked to a particular commit hash or anything.

npendery added in progress needs clarification question and removed needs clarification question labels May 7, 2017

begedin reviewed May 8, 2017

View reviewed changes

npendery added needs code review and removed in progress labels May 16, 2017

begedin reviewed May 17, 2017

View reviewed changes

npendery added ready to merge and removed needs code review labels May 31, 2017

npendery force-pushed the np-github-create-task branch 2 times, most recently from a486932 to fca52bf Compare June 1, 2017 21:03

begedin mentioned this pull request Jul 20, 2017

GitHub update task #806

Closed

2 tasks

begedin force-pushed the np-github-create-task branch from ed17814 to 945277c Compare August 9, 2017 14:31

begedin removed the ready to merge label Aug 14, 2017

begedin mentioned this pull request Aug 18, 2017

Added github-repo-select component code-corps/code-corps-ember#1295

Merged

begedin mentioned this pull request Aug 29, 2017

Remove ja_resource #864

Closed

begedin changed the title ~~Add ability to create issue in github for relative task~~ Connect task with github on creation, if applicable Aug 29, 2017

begedin force-pushed the np-github-create-task branch 2 times, most recently from da7d128 to e6baaaf Compare August 29, 2017 14:11

begedin force-pushed the np-github-create-task branch from e6f5493 to b4ee66f Compare September 8, 2017 05:27

begedin force-pushed the np-github-create-task branch from b4ee66f to ab09f38 Compare September 11, 2017 12:36

begedin mentioned this pull request Sep 11, 2017

Added comment syncing with github during create and update #925

Merged

begedin force-pushed the np-github-create-task branch 2 times, most recently from 0cf146d to 0261a04 Compare September 12, 2017 13:48

begedin changed the title ~~Connect task with github on creation, if applicable~~ Integration of Tasks with GitHub issues Sep 13, 2017

begedin mentioned this pull request Sep 13, 2017

Added missing tests for user controller github connect #930

Merged

begedin force-pushed the np-github-create-task branch from d138594 to 8e9e980 Compare September 14, 2017 06:42

begedin mentioned this pull request Sep 14, 2017

Cleaned up ProjectController, introduced Project.Query #934

Merged

begedin force-pushed the np-github-create-task branch 2 times, most recently from 0b0ec6d to f494d39 Compare September 14, 2017 15:00

begedin mentioned this pull request Sep 14, 2017

[USERSKILL]: refactor away ja_resource #926

Merged

joshsmith reviewed Sep 14, 2017

View reviewed changes

joshsmith force-pushed the np-github-create-task branch from f494d39 to d13b138 Compare September 14, 2017 22:09

This was referenced Sep 15, 2017

When creating a user from github, merge github info into user info #938

Closed

TaskSyncer.sync_all can return error changesets that get swallowed by our GitHub issues event handler without error #940

Closed

npendery and others added 4 commits September 15, 2017 11:13

Added CodeCorps.Plug.IdsToIntegers

12d4e4d

Added proper naming to number attribute on task route, for clearer ta…

4b5aed9

…sk query

Added logging for errors, some error handling, and fix for timestamp …

ce0c874

…expiration

begedin force-pushed the np-github-create-task branch from 8db93a3 to ce0c874 Compare September 15, 2017 09:22

joshsmith approved these changes Sep 15, 2017

View reviewed changes

joshsmith merged commit a8c204e into develop Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration of Tasks with GitHub issues #805

Integration of Tasks with GitHub issues #805

npendery commented May 7, 2017 •

edited by begedin

Loading

begedin May 8, 2017

npendery May 9, 2017

npendery May 10, 2017

begedin May 8, 2017

npendery May 9, 2017

begedin May 10, 2017

npendery May 10, 2017

begedin May 8, 2017

npendery commented May 16, 2017

begedin May 17, 2017

begedin May 17, 2017

begedin May 17, 2017

npendery May 18, 2017

begedin commented May 17, 2017

joshsmith commented May 25, 2017

begedin commented Jul 20, 2017 •

edited

Loading

begedin commented Aug 9, 2017

begedin commented Aug 14, 2017

joshsmith commented Aug 17, 2017

begedin commented Aug 17, 2017 •

edited

Loading

begedin commented Aug 29, 2017

begedin commented Sep 6, 2017

joshsmith commented Sep 7, 2017

begedin commented Sep 8, 2017

joshsmith Sep 14, 2017

joshsmith commented Sep 15, 2017 •

edited by begedin

Loading

begedin commented Sep 15, 2017

begedin commented Sep 15, 2017

joshsmith commented Sep 15, 2017

Integration of Tasks with GitHub issues #805

Integration of Tasks with GitHub issues #805

Conversation

npendery commented May 7, 2017 • edited by begedin Loading

What's in this PR?

To do

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npendery commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented May 17, 2017

joshsmith commented May 25, 2017

begedin commented Jul 20, 2017 • edited Loading

begedin commented Aug 9, 2017

begedin commented Aug 14, 2017

joshsmith commented Aug 17, 2017

begedin commented Aug 17, 2017 • edited Loading

begedin commented Aug 29, 2017

Considerations

Approaches for mocking

begedin commented Sep 6, 2017

joshsmith commented Sep 7, 2017

begedin commented Sep 8, 2017

Choose a reason for hiding this comment

joshsmith commented Sep 15, 2017 • edited by begedin Loading

begedin commented Sep 15, 2017

begedin commented Sep 15, 2017

joshsmith commented Sep 15, 2017

npendery commented May 7, 2017 •

edited by begedin

Loading

begedin commented Jul 20, 2017 •

edited

Loading

begedin commented Aug 17, 2017 •

edited

Loading

joshsmith commented Sep 15, 2017 •

edited by begedin

Loading