Add GitHub user information

[joshsmith]

What's in this PR?

Fetches the GitHub user from the GitHub API and save the information to the database.

Still needs:

• Tests
• Some refactoring (don't like the way the surface area of the GitHub API modules look right now)
• Add missing indexes

[begedin]

This could be made into a struct, for a bit more "typechecking", using defstruct or even using an ecto embedded schema.

[begedin]

With the amount of fields now, a separate model might not be a terrible idea.

[begedin]

@joshsmith I'm not sure of the move of lib/code_corps/github.ex to lib/code_corps/github/github.ex

1. The module name is CodeCorps.Github, not CodeCorps.Github.Github
2. From the phoenix 1.3 talk:

It seems like the file organizing philosophy goes towards having it in lib/code_corps/github.ex

[joshsmith]

@begedin would really love your thoughts on this. Took a first stab at implementing with Bypass here (been meaning to try it with stripity_stripe for some time now). I really liked your previous implementation with @behaviour but wanted to try a slightly different approach that might need less setup going forward. Clearly I have not quite solved that problem, but feel like you might have some insight here after seeing these changes. :)

[joshsmith]

I am not super happy with how verbose I have to be here. I'd love for this to be reduced to a couple lines here somehow.

[joshsmith]

All of this is in violation of DRY, just copied/pasted from below.

[joshsmith]

I think maybe this could all just be folded into one config rather than duplicated across them.

[begedin]

We could use config.ex as the base, then override for environments where it needs to be overriden.

[joshsmith]

Best way I could think to allow us to inject a different Bypass server endpoint, borrowed from an article on Bypass.

[joshsmith]

This needs to be done still. Not sure best way to handle the RSA key portion, either.

[joshsmith]

Went with this as the place to handle all our top-level OAuth requests.

[joshsmith]

This will need more functions obviously.

[joshsmith]

This might be simplified with struct recommendation above.

[joshsmith]

Not sure if we could have a different case for this, or some other way to use more quickly this setup in tests in the future without copying/pasting everywhere.

[begedin]

We could use config.ex as the base, then override for environments where it needs to be overriden.

[begedin]

I would love if this would work, but it actually means the string will contain an \n after "with". I've been looking for a way to nicely split a string across multiple lines without actually introducing newline characters, and best I could figure out was:

"The GitHub HTTP client encountered an error while communicating with " <>
"the GitHub API."

Note the required space after the "with".

[begedin]

This is almost done. What's left is to

• get a private key file
• get the app id

[begedin]

This is placeholder until we get a .pem file of our own

[begedin]

If were to delay decoding the response and instead send the encoded body out, our individual module endpoints (such as Github.User could automatically cast as structs:

defmodule Person do
  @derive [Poison.Encoder]
  defstruct [:name, :age]
end

Poison.encode!(%Person{name: "Devin Torres", age: 27})
#=> "{\"name\":\"Devin Torres\",\"age\":27}"

Poison.decode!(~s({"name": "Devin Torres", "age": 27}), as: %Person{})

I'm not really sure we're gaining enough that way to do it, though

[DavidAntaramian]

I was asked to provide some input on this in the context of separating business logic from the Github API integration. We've been working through similar problem at Timber.io, so I'm happy to share some advice based on our experiences.

As you know, some of the initial stripity_stripe 2.0 code I provided you was basically ripped from our billing component application. Our business level code in that application that deals with customer accounts, subscriptions, etc., is tightly coupled with the Stripe HTTP layer. I regret that every time I have to write or update tests for that application, now. The tests for that application utilize Bypass, because that's the only way to test it currently, but it's far from an ideal situation. The Bypass blocks take up more room than they should, and they're needed even when the test should just be a unit test.

What does that mean for this?

One of the things that I'm doing in some new code for Timber.io is implementing the recommendations of Mocks and Contracts more stringently than I was prior to avoid the situation described above, and I think I can recommend the same thing for you. I'm doing this by separating the line-of-business work from the integration work.

For example, retrieving users from GitHub:

defmodule CodeCorps.GitHub.User do
  defstruct [:avatar_url, :email, :id, :login]

  def me(access_token, opts \\ []) do
     github_client = get_github_client(opts)
     {:ok, user} = github_client.get_user(access_token)
     
     api_response_to_user(user)
  end

  defp get_github_client(opts) do
    # Fetch the GitHub client; if one is not provided in the opts list,
    # then get the default from the application configuration, otherwise,
    # fallback to the HTTP client
    case Keyword.get(opts, :github_client) do
      nil ->
       Application.get(:code_corps, __MODULE__, [])
       |> Keyword.get(:github_client, CodeCorps.GitHub.HTTPClient)   
      github_client ->
       github_client
    end
  end
end

defmodule CodeCorps.GitHub.Client do
  @callback get_user(access_token) :: {:ok, map} | {:error, reason}
end

defmodule CodeCorps.GitHub.HTTPClient do
  @behaviour CodeCorps.Github.Client

  ## all that fun "real" implementation
end

defmodule CodeCorps.GitHub.InMemoryClient do
  @behaviour CodeCorps.GitHub.Client
  
  # either make a simple GenServer that allows setting and
  # retrieving data or just return static data all the time
end

The pros of this are:

• You can test GitHub.User and GitHub.HTTPClient separately
• GitHub.HTTPClient can be tested using Bypass, while Github.User can be tested using the GitHub.InMemoryClient
• You can override the functionality in tests as needed

The cons are:

• It obviously increases the amount of code you'll write

What I'm trying to do is improve this further by allowing a function to be passed to opts and taking either that or the default one. I haven't worked that out entirely yet, but here's a rough version:

defmodule CodeCorps.GitHub.User do
  defstruct [:avatar_url, :email, :id, :login]

  def me(access_token, opts \\ []) do
     get_user = get_implementation(opts, &CodeCorps.GitHub.HTTPClient.get_user/1)
     {:ok, user} = get_user.(access_token)
     
     api_response_to_user(user)
  end

  defp get_implementation(opts, default) do
    Keyword.get(opts, :implementation, default)
  end
end

The code here might seem much more convoluted, but it allows you to do things like this in testing:

alias CodeCorps.GitHub.User

test "retrieves user from GitHub " do
  user_login = "davidantaramian"

  get_user = fn (_) ->
    {:ok, %{"login" => user_login, "id" => …}}
  end

  user = User.me("abcdefg")

  assert user.login == user_login
end

I hope that this helps you on figuring out a preferred pathway. I recommend loosely coupling wherever possible to avoid testing hell. As of yet, though, I wouldn't say there's a "best way to do it."

[begedin]

I assume you meant to use &from_pem_file/1 here.

It's not well documented how &from_pem/1 works. I tried copying the file content into an environment variables in various ways, but none of them worked.

Basically, could you clarify? Do we

• store the contents of the .pem file into the environment and use &from_pem/1
• store the path to the .pem file into the environment and use &from_pem_file/1

[DavidAntaramian]

I'm going to put a further two cents here, since I'm looking this over. I would also recommend against using a module attribute here (@rsa_key). This prevents dynamic resolution of the file location. Even running in Docker, just changing an environment variable and restarting the application will not be enough to trigger recompile of the module to pick up the new value. You will either need to launch a new Docker container which compiles the code without any previous build artifacts or force compilation on the existing container.

Instead, prefer functions like the following:

defp get_rsa_file_location() do
  Application.get_env(:code_corps, :github_app_pem)
end

This prevents the file location from being "burned in" at build time, since the configuration information is fetched at runtime instead of at build time.

[joshsmith]

We store the contents. You'll see I added a GITHUB_APP_PEM, which is maybe a misnomer. This will be the contents of the RSA private key, and on Heroku will also be stored as text. I believe Heroku allows up to 32kb of total data in your config vars.

[DavidAntaramian]

Yes, 32kb total which includes both the keys and their values.