[GITHUB]: connect #807

snewcomer · 2017-05-11T21:14:22Z

This PR is meant to retrieving an access token for a user and updating the User record with their github_id.

Not sure where constants @token_url, @client_id, @client_secret come from.

Progress on: #789

snewcomer · 2017-05-11T21:15:02Z

lib/code_corps/github.ex

+  end
+
+  defp update_user(%{github_id: _github_id} = github_info, user) do
+    associate(user, github_info)


Maybe just pass the github_id instead of the whole map?

You mean, pass into associate/2? That would work for now, if that's the only thing we need to pass in. Later on, we might not have just the client ID but instead store a larger set of values, but we can worry about that when we come to it.

That being said, if associate already exists, and tests for it exist, no point i changing it now.

snewcomer · 2017-05-11T21:15:27Z

lib/code_corps/github.ex

+  @token_url ""
+  @client_id ""
+  @client_secret ""
+


Need help with these.

client_id and _secret will be environment variables which we will get when we register CodeCorps as a github application. It may even make sense not to have them as module variables, so we can inject them in tests.

Either way, assume the environment variables GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET exist, are loaded during config and stored into a config key.

For @token_url, this should simply be https://github.com/login/oauth/access_token

snewcomer · 2017-05-12T03:44:23Z

Just looking for some initial feedback and help! Haven't looked at testing, b/c wanted to see first if going down the right path.

begedin · 2017-05-15T13:48:00Z

@snewcomer I'm currently in the process of implementing the API key stuff as part of this PR, since, in order to do that, I need to work with Github API keys for CodeCorps. I'll try to implement it in a way so that tests will be reliable, but will not require other developers to provide API keys. Be sure to pull the latest before doing more work on this. I'll make sure to do the same.

begedin · 2017-05-15T16:31:14Z

@snewcomer My commit still needs thorough testing and wrapping up, but I managed to get through the basic flow.

Renamed user.github_id to user.github_auth_token, since that's a more accurate name.
Extracted the actual API calls into a separate module, so we can inject a test module in tests instead.
For testing, you can use the test module.
In dev, you can temporarily set the test module as the one being used, or you can register a github oauth app on your github profile, add those keys to your .env file (look at .env.example to see what to name them, then run source .env from bash before running the app.
At that point, you'd need to
- visit https://github.com/login/oauth/authorize?scope=user:email&client_id=<your_client_id> to get a code
- use that code when calling Github.connect(user, code)
  If you can work around this, feel free. Otherwise, I'll be able to wrap it up tomorrow.

snewcomer

So guessing we can remove the tentacat dep? These changes look great!

snewcomer · 2017-05-16T16:09:24Z

lib/code_corps/github/api.ex

+  }
+
+  def connect(code) do
+    with {:ok, %HTTPoison.Response{body: response}} <- code |> build_connect_params() |> do_connect(),


@begedin Just curious on the reason not to use Tentacat.

I had issues getting it to work. The documented methods do not work as documented. They also raise exceptions when they should not be doing so and are named without exclamation marks.

Due to that, for this specific case, I went with a direct API call.

For the regular stuff, in the integration milestone, we should go with Tentacat.

snewcomer · 2017-05-16T16:10:18Z

lib/code_corps/github.ex

-    |> Tentacat.Client.new
-    |> Tentacat.Users.me
-    |> update_user(user)
+    case code |> @api.connect do


Yeah I like this move to a separate module for testing

joshsmith

Minor comments that I think deserve some work before a merge.

joshsmith · 2017-05-17T20:43:04Z

lib/code_corps/github/api.ex

+    do
+      {:ok, access_token}
+    else
+      {:ok, %{"error" => error}} -> {:error, error}


It's a bit weird to me that this returns an :ok tuple.

Yeah, surprised a HTTPoison request with an error response would return an :ok tuple, there should be a better way of differentiating a HTTPoison error and a response error

It is what it is, though. Seems that github itself returns a 200 ok, with an error hash. Not much we can do about it, other than interpret it here.

joshsmith · 2017-05-17T20:44:14Z

priv/repo/migrations/20170515154812_rename_user_github_id_to_github_auth_token.exs

+  use Ecto.Migration
+
+  def change do
+    rename table(:users), :github_id, to: :github_auth_token


Are you no longer expecting to track the GitHub user ID? I would think this would be important information for us to have on-hand and indexed.

I did not consider that. We'd need it as part of the integration feature. Technically, however, all the connect feature needs is the auth_token.

I'll revert this change and make it so both fields are there.

joshsmith · 2017-05-17T20:45:03Z

web/models/user.ex

@@ -97,8 +98,8 @@ defmodule CodeCorps.User do

  def github_associate_changeset(struct, params) do


Why not github_association changeset, since it looks like this wants to be a noun and associate in this case is verbed?

joshsmith · 2017-05-25T21:34:40Z

@snewcomer @begedin are we able to implement the above minor suggestions and then get this merged?

begedin

@joshsmith Made the changes, rebased and squashed. This ought to be good to merge.

I've added support for an optional parameter for the connect method, so we can explicityl inject a boundary module when testing. This allows really easy testing for various response cases and is advised to do in http://blog.plataformatec.com.br/2015/10/mocks-and-explicit-contracts/

Outdated

snewcomer added needs clarification needs work question labels May 11, 2017

snewcomer self-assigned this May 11, 2017

snewcomer requested a review from begedin May 11, 2017 21:14

snewcomer commented May 11, 2017

View reviewed changes

snewcomer changed the title ~~[GITHUB]: connect initial with no tests~~ [GITHUB]: connect May 11, 2017

snewcomer commented May 16, 2017

View reviewed changes

joshsmith previously requested changes May 17, 2017

View reviewed changes

joshsmith removed the needs clarification label May 17, 2017

begedin force-pushed the github-connect branch 3 times, most recently from 0dc0c24 to 2c78d03 Compare May 26, 2017 10:20

begedin approved these changes May 26, 2017

View reviewed changes

joshsmith force-pushed the github-connect branch from 2c78d03 to 80cce56 Compare May 30, 2017 23:57

joshsmith added ready to merge and removed needs work question labels May 30, 2017

Added Github connect implementation

2edca91

joshsmith force-pushed the github-connect branch from 80cce56 to 2edca91 Compare May 31, 2017 21:20

joshsmith merged commit e072844 into develop May 31, 2017

joshsmith deleted the github-connect branch May 31, 2017 21:24

begedin mentioned this pull request Sep 13, 2017

Add CodeCorps.Github module with connect/2 function #789

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GITHUB]: connect #807

[GITHUB]: connect #807

snewcomer commented May 11, 2017

snewcomer May 11, 2017

begedin May 12, 2017

snewcomer May 11, 2017

begedin May 12, 2017

snewcomer commented May 12, 2017

begedin commented May 15, 2017

begedin commented May 15, 2017

snewcomer left a comment

snewcomer May 16, 2017

begedin May 16, 2017

snewcomer May 16, 2017

joshsmith left a comment

joshsmith May 17, 2017

npendery May 18, 2017 •

edited

begedin May 18, 2017

joshsmith May 17, 2017

begedin May 18, 2017 •

edited

joshsmith May 17, 2017

joshsmith commented May 25, 2017

begedin left a comment

		@@ -97,8 +98,8 @@ defmodule CodeCorps.User do

		def github_associate_changeset(struct, params) do

[GITHUB]: connect #807

[GITHUB]: connect #807

Conversation

snewcomer commented May 11, 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

snewcomer commented May 12, 2017

begedin commented May 15, 2017

begedin commented May 15, 2017

snewcomer left a 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

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npendery May 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin May 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented May 25, 2017

begedin left a comment

Choose a reason for hiding this comment

npendery May 18, 2017 •

edited

begedin May 18, 2017 •

edited