From 2edca9149571d5d8d1a36b149f8bb04f4876cf19 Mon Sep 17 00:00:00 2001 From: snewcomer Date: Thu, 11 May 2017 14:06:40 -0700 Subject: [PATCH 1/4] Added Github connect implementation --- .env.example | 2 + config/dev.exs | 6 +++ config/prod.exs | 4 ++ config/remote-development.exs | 4 ++ config/staging.exs | 4 ++ config/test.exs | 2 + lib/code_corps/github.ex | 34 ++++++++++++-- lib/code_corps/github/api.ex | 44 ++++++++++++++++++ lib/code_corps/github/api_contract.ex | 17 +++++++ lib/code_corps/github/test_api.ex | 13 ++++++ mix.exs | 2 + mix.lock | 1 + ...6095401_add_github_auth_token_to_users.exs | 9 ++++ priv/repo/structure.sql | 6 +-- test/controllers/user_controller_test.exs | 2 +- test/lib/code_corps/github_test.exs | 46 ++++++++++++++++--- test/models/user_test.exs | 12 ++--- web/controllers/user_controller.ex | 2 +- web/models/project.ex | 2 + web/models/user.ex | 10 ++-- 20 files changed, 196 insertions(+), 26 deletions(-) create mode 100644 lib/code_corps/github/api.ex create mode 100644 lib/code_corps/github/api_contract.ex create mode 100644 lib/code_corps/github/test_api.ex create mode 100644 priv/repo/migrations/20170526095401_add_github_auth_token_to_users.exs diff --git a/.env.example b/.env.example index 4e3766b95..1ae8373fd 100644 --- a/.env.example +++ b/.env.example @@ -4,6 +4,8 @@ export CLOUDEX_API_KEY= export CLOUDEX_CLOUD_NAME= export CLOUDEX_SECRET= export CLOUDFRONT_DOMAIN= +export GITHUB_CLIENT_ID= +export GITHUB_CLIENT_SECRET= export POSTMARK_API_KEY= export S3_BUCKET= export SEGMENT_WRITE_KEY= diff --git a/config/dev.exs b/config/dev.exs index c36faaf54..74e0300cf 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -49,6 +49,8 @@ config :guardian, Guardian, config :code_corps, :analytics, CodeCorps.Analytics.InMemoryAPI +config :code_corps, :github_api, CodeCorps.Github.API + # Configures stripe for dev mode config :code_corps, :stripe, Stripe config :code_corps, :stripe_env, :dev @@ -71,3 +73,7 @@ if System.get_env("CLOUDEX_API_KEY") == nil do config :code_corps, :cloudex, CloudexTest config :cloudex, api_key: "test_key", secret: "test_secret", cloud_name: "test_cloud_name" end + +config :code_corps, + github_client_id: System.get_env("GITHUB_CLIENT_ID"), + github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") diff --git a/config/prod.exs b/config/prod.exs index 5959dcb41..c5d375f7c 100644 --- a/config/prod.exs +++ b/config/prod.exs @@ -65,6 +65,10 @@ config :code_corps, postmark_project_acceptance_template: "1447041", postmark_receipt_template: "1255222" +config :code_corps, + github_client_id: System.get_env("GITHUB_CLIENT_ID"), + github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + # ## SSL Support # # To get SSL working, you will need to add the `https` key diff --git a/config/remote-development.exs b/config/remote-development.exs index be64e416d..4df1d6924 100644 --- a/config/remote-development.exs +++ b/config/remote-development.exs @@ -47,6 +47,10 @@ config :code_corps, postmark_project_acceptance_template: "123", postmark_receipt_template: "123" +config :code_corps, + github_client_id: System.get_env("GITHUB_CLIENT_ID"), + github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + # ## SSL Support # # To get SSL working, you will need to add the `https` key diff --git a/config/staging.exs b/config/staging.exs index 6e8a48c83..8fd031fd9 100644 --- a/config/staging.exs +++ b/config/staging.exs @@ -63,6 +63,10 @@ config :code_corps, postmark_project_acceptance_template: "1447022", postmark_receipt_template: "1252361" +config :code_corps, + github_client_id: System.get_env("GITHUB_CLIENT_ID"), + github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + # ## SSL Support # # To get SSL working, you will need to add the `https` key diff --git a/config/test.exs b/config/test.exs index 20ba08cd1..e6a3a2be5 100644 --- a/config/test.exs +++ b/config/test.exs @@ -32,6 +32,8 @@ config :guardian, Guardian, config :code_corps, :analytics, CodeCorps.Analytics.TestAPI +config :code_corps, :github_api, CodeCorps.Github.TestAPI + # Configures stripe for test mode config :code_corps, :stripe, CodeCorps.StripeTesting config :code_corps, :stripe_env, :test diff --git a/lib/code_corps/github.ex b/lib/code_corps/github.ex index 6f54622a6..1101dc04e 100644 --- a/lib/code_corps/github.ex +++ b/lib/code_corps/github.ex @@ -1,15 +1,41 @@ defmodule CodeCorps.Github do - alias CodeCorps.{User, Repo} + @api Application.get_env(:code_corps, :github_api) + @doc """ - Temporary function until the actual behavior is implemented. + Posts code to github to receive an auth token, associates user with that + auth token. + + Accepts a third parameter, which is a custom API module, for the purposes of + explicit dependency injection during testing. + + Returns one of the following: + + - {:ok, %CodeCorps.User{}} + - {:error, %Ecto.Changeset{}} + - {:error, "some_github_error"} """ - def connect(user, _code), do: {:ok, user} + @spec connect(User.t, String.t, module) :: {:ok, User.t} | {:error, String.t} + def connect(%User{} = user, code, api \\ @api) do + case code |> api.connect do + {:ok, github_auth_token} -> user |> associate(%{github_auth_token: github_auth_token}) + {:error, error} -> {:error, error} + end + end + + @doc """ + Associates user with an auth token + Returns one of the following: + + - {:ok, %CodeCorps.User{}} + - {:error, %Ecto.Changeset{}} + """ + @spec associate(User.t, map) :: {:ok, User.t} | {:error, Ecto.Changeset.t} def associate(user, params) do user - |> User.github_associate_changeset(params) + |> User.github_association_changeset(params) |> Repo.update() end end diff --git a/lib/code_corps/github/api.ex b/lib/code_corps/github/api.ex new file mode 100644 index 000000000..f61107fe3 --- /dev/null +++ b/lib/code_corps/github/api.ex @@ -0,0 +1,44 @@ +defmodule CodeCorps.Github.API do + @moduledoc """ + The boundary module which communicates with the Github API using either + direct requests, or through Tentacat + """ + @behaviour CodeCorps.Github.APIContract + + @client_secret Application.get_env(:code_corps, :github_client_secret) + @client_id Application.get_env(:code_corps, :github_client_id) + + @base_connect_params %{ + client_id: @client_id, + client_secret: @client_secret + } + + @doc """ + Receives a code generated through the client-side github connect process and + posts it to github. + + Returns either an {:ok, access_token}, or an {:error, error_message}. + """ + @spec connect(String.t) :: {:ok, String.t} | {:error, String.t} + def connect(code) do + with {:ok, %HTTPoison.Response{body: response}} <- code |> build_connect_params() |> do_connect(), + {:ok, %{"access_token" => access_token}} <- response |> Poison.decode + do + {:ok, access_token} + else + {:ok, %{"error" => error}} -> {:error, error} + end + end + + @connect_url "https://github.com/login/oauth/access_token" + + @spec do_connect(map) :: {:ok, HTTPoison.Response.t | HTTPoison.AsyncResponse.t} | {:error, HTTPoison.Error.t} + defp do_connect(params) do + HTTPoison.post(@connect_url, "", [{"Accept", "application/json"}], [params: params]) + end + + @spec build_connect_params(String.t) :: map + defp build_connect_params(code) do + @base_connect_params |> Map.put(:code, code) + end +end diff --git a/lib/code_corps/github/api_contract.ex b/lib/code_corps/github/api_contract.ex new file mode 100644 index 000000000..deb1aa4f5 --- /dev/null +++ b/lib/code_corps/github/api_contract.ex @@ -0,0 +1,17 @@ +defmodule CodeCorps.Github.APIContract do + @moduledoc """ + Defines a contract for a github API module, listing all functions the module + should implement. + + This contract should be specified as behaviour for the default `Github.API` + module, as well as any custom module we inject in tests. + """ + + @doc """ + Receives a code string, created in the client part of the github connect process, + returns either an :ok tupple indicating a successful connect process, where + the second element is the auth token string, or an :error tuple, where the + second element is an error message, or a struct + """ + @callback connect(code :: String.t) :: {:ok, auth_token :: String.t} | {:error, error :: String.t} +end diff --git a/lib/code_corps/github/test_api.ex b/lib/code_corps/github/test_api.ex new file mode 100644 index 000000000..d248591be --- /dev/null +++ b/lib/code_corps/github/test_api.ex @@ -0,0 +1,13 @@ +defmodule CodeCorps.Github.TestAPI do + @moduledoc """ + The most basic implementation of an API module for testing. All functions + here should return successes. + + If we want to test the wrapper module, we can specify custom API modules + during function calls. + """ + @behaviour CodeCorps.Github.APIContract + + @spec connect(String.t) :: {:ok, String.t} + def connect(code), do: send(self(), {:ok, code}) +end diff --git a/mix.exs b/mix.exs index 9e20641fe..b09dec7d9 100644 --- a/mix.exs +++ b/mix.exs @@ -45,6 +45,7 @@ defmodule CodeCorps.Mixfile do :segment, :sentry, :stripity_stripe, + :tentacat, :timber, :timex_ecto ] @@ -96,6 +97,7 @@ defmodule CodeCorps.Mixfile do {:sentry, "~> 2.0"}, # Sentry error tracking {:stripity_stripe, git: "https://github.com/code-corps/stripity_stripe.git", branch: "2.0"}, # Stripe {:sweet_xml, "~> 0.5"}, + {:tentacat, "~> 0.5"}, {:timber, "~> 0.4"}, # Logging {:timex, "~> 3.0"}, {:timex_ecto, "~> 3.0"}, diff --git a/mix.lock b/mix.lock index 1f8d63ff0..bc5f94354 100644 --- a/mix.lock +++ b/mix.lock @@ -59,6 +59,7 @@ "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], []}, "stripity_stripe": {:git, "https://github.com/code-corps/stripity_stripe.git", "1e9b87d73710ef901dd6d735d6f2d0a2cef75d1c", [branch: "2.0"]}, "sweet_xml": {:hex, :sweet_xml, "0.6.5", "dd9cde443212b505d1b5f9758feb2000e66a14d3c449f04c572f3048c66e6697", [:mix], []}, + "tentacat": {:hex, :tentacat, "0.6.2", "e5e5ad95d577dd441e4dcfcab259c9d92b0049f0481a4be6453769d61a956a3b", [:mix], [{:exjsx, "~> 3.2", [hex: :exjsx, optional: false]}, {:httpoison, "~> 0.8", [hex: :httpoison, optional: false]}]}, "timber": {:hex, :timber, "0.4.7", "df3fcd79bcb4eb4b53874d906ef5f3a212937b4bc7b7c5b244745202cc389443", [:mix], [{:ecto, "~> 2.0", [hex: :ecto, optional: true]}, {:phoenix, "~> 1.2", [hex: :phoenix, optional: true]}, {:plug, "~> 1.2", [hex: :plug, optional: true]}, {:poison, "~> 2.0 or ~> 3.0", [hex: :poison, optional: false]}]}, "timex": {:hex, :timex, "3.0.8", "71d5ebafcdc557c6c866cdc196c5054f587e7cd1118ad8937e2293d51fc85608", [:mix], [{:combine, "~> 0.7", [hex: :combine, optional: false]}, {:gettext, "~> 0.10", [hex: :gettext, optional: false]}, {:tzdata, "~> 0.1.8 or ~> 0.5", [hex: :tzdata, optional: false]}]}, "timex_ecto": {:hex, :timex_ecto, "3.1.1", "37d54f6879d96a6789bb497296531cfb853631de78e152969d95cff03c1368dd", [:mix], [{:ecto, "~> 2.1.0", [hex: :ecto, optional: false]}, {:timex, "~> 3.0", [hex: :timex, optional: false]}]}, diff --git a/priv/repo/migrations/20170526095401_add_github_auth_token_to_users.exs b/priv/repo/migrations/20170526095401_add_github_auth_token_to_users.exs new file mode 100644 index 000000000..51a0a216f --- /dev/null +++ b/priv/repo/migrations/20170526095401_add_github_auth_token_to_users.exs @@ -0,0 +1,9 @@ +defmodule CodeCorps.Repo.Migrations.AddGithubAuthTokenToUsers do + use Ecto.Migration + + def change do + alter table(:users) do + add :github_auth_token, :string + end + end +end diff --git a/priv/repo/structure.sql b/priv/repo/structure.sql index 42634eed9..56a1ba05b 100644 --- a/priv/repo/structure.sql +++ b/priv/repo/structure.sql @@ -1324,7 +1324,8 @@ CREATE TABLE users ( cloudinary_public_id character varying(255), default_color character varying(255), sign_up_context character varying(255) DEFAULT 'default'::character varying, - github_id character varying(255) + github_id character varying(255), + github_auth_token character varying(255) ); @@ -2598,5 +2599,4 @@ ALTER TABLE ONLY user_tasks -- PostgreSQL database dump complete -- -INSERT INTO "schema_migrations" (version) VALUES (20160723215749), (20160804000000), (20160804001111), (20160805132301), (20160805203929), (20160808143454), (20160809214736), (20160810124357), (20160815125009), (20160815143002), (20160816020347), (20160816034021), (20160817220118), (20160818000944), (20160818132546), (20160820113856), (20160820164905), (20160822002438), (20160822004056), (20160822011624), (20160822020401), (20160822044612), (20160830081224), (20160830224802), (20160911233738), (20160912002705), (20160912145957), (20160918003206), (20160928232404), (20161003185918), (20161019090945), (20161019110737), (20161020144622), (20161021131026), (20161031001615), (20161121005339), (20161121014050), (20161121043941), (20161121045709), (20161122015942), (20161123081114), (20161123150943), (20161124085742), (20161125200620), (20161126045705), (20161127054559), (20161205024856), (20161207112519), (20161209192504), (20161212005641), (20161214005935), (20161215052051), (20161216051447), (20161218005913), (20161219160401), (20161219163909), (20161220141753), (20161221085759), (20161226213600), (20161231063614), (20170102130055), (20170102181053), (20170104113708), (20170104212623), (20170104235423), (20170106013143), (20170115035159), (20170115230549), (20170121014100), (20170131234029), (20170201014901), (20170201025454), (20170201035458), (20170201183258), (20170220032224), (20170224233516), (20170226050552), (20170228085250), (20170308214128), (20170308220713), (20170308222552), (20170313130611), (20170318032449), (20170318082740), (20170324194827), (20170424215355), (20170501225441); - +INSERT INTO "schema_migrations" (version) VALUES (20160723215749), (20160804000000), (20160804001111), (20160805132301), (20160805203929), (20160808143454), (20160809214736), (20160810124357), (20160815125009), (20160815143002), (20160816020347), (20160816034021), (20160817220118), (20160818000944), (20160818132546), (20160820113856), (20160820164905), (20160822002438), (20160822004056), (20160822011624), (20160822020401), (20160822044612), (20160830081224), (20160830224802), (20160911233738), (20160912002705), (20160912145957), (20160918003206), (20160928232404), (20161003185918), (20161019090945), (20161019110737), (20161020144622), (20161021131026), (20161031001615), (20161121005339), (20161121014050), (20161121043941), (20161121045709), (20161122015942), (20161123081114), (20161123150943), (20161124085742), (20161125200620), (20161126045705), (20161127054559), (20161205024856), (20161207112519), (20161209192504), (20161212005641), (20161214005935), (20161215052051), (20161216051447), (20161218005913), (20161219160401), (20161219163909), (20161220141753), (20161221085759), (20161226213600), (20161231063614), (20170102130055), (20170102181053), (20170104113708), (20170104212623), (20170104235423), (20170106013143), (20170115035159), (20170115230549), (20170121014100), (20170131234029), (20170201014901), (20170201025454), (20170201035458), (20170201183258), (20170220032224), (20170224233516), (20170226050552), (20170228085250), (20170308214128), (20170308220713), (20170308222552), (20170313130611), (20170318032449), (20170318082740), (20170324194827), (20170424215355), (20170501225441), (20170526095401); diff --git a/test/controllers/user_controller_test.exs b/test/controllers/user_controller_test.exs index 483baf943..9baed8590 100644 --- a/test/controllers/user_controller_test.exs +++ b/test/controllers/user_controller_test.exs @@ -246,7 +246,7 @@ defmodule CodeCorps.UserControllerTest do test "return the user when current user connects successfully", %{conn: conn} do user = insert(:user) - code = %{"code" => "client generated code"} + code = %{"code" => "valid_code"} path = user_path(conn, :github_connect, code) diff --git a/test/lib/code_corps/github_test.exs b/test/lib/code_corps/github_test.exs index a1b5c31b4..c8a1a8657 100644 --- a/test/lib/code_corps/github_test.exs +++ b/test/lib/code_corps/github_test.exs @@ -1,20 +1,52 @@ defmodule CodeCorps.GithubTest do use CodeCorps.ModelCase - alias CodeCorps.Github + + alias CodeCorps.{ + Github, User + } + + alias Ecto.Changeset describe "associate/2" do - test "should update the user with the github_id" do + test "updates the user, returns :ok tuple with user" do user = insert(:user) - params = %{github_id: "foobar"} - {:ok, result} = Github.associate(user, params) - assert result.github_id == "foobar" + params = %{github_auth_token: "foobar"} + {:ok, %User{} = returned_user} = Github.associate(user, params) + assert user.id == returned_user.id + assert returned_user.github_auth_token == "foobar" end - test "should return the error with a changeset" do + test "returns :error tupple with changeset if there are validation errors" do user = insert(:user) params = %{} - {:error, changeset} = Github.associate(user, params) + {:error, %Changeset{} = changeset} = Github.associate(user, params) refute changeset.valid? end end + + defmodule SuccessAPI do + @behaviour CodeCorps.Github.APIContract + def connect(_code), do: {:ok, "foo_auth_token"} + end + + defmodule ErrorAPI do + @behaviour CodeCorps.Github.APIContract + def connect(_code), do: {:error, "foo_error"} + end + + describe "connect/2" do + test "posts to github, updates user if reply is ok, returns updated user" do + user = insert(:user) + + {:ok, %User{} = returned_user} = Github.connect(user, "foo", SuccessAPI) + + assert returned_user.id == user.id + assert returned_user.github_auth_token == "foo_auth_token" + end + + test "posts to github, returns error if reply is not ok" do + user = insert(:user) + assert {:error, "foo_error"} == Github.connect(user, "foo", ErrorAPI) + end + end end diff --git a/test/models/user_test.exs b/test/models/user_test.exs index 23a63eb16..a8fbfdd81 100644 --- a/test/models/user_test.exs +++ b/test/models/user_test.exs @@ -140,16 +140,16 @@ defmodule CodeCorps.UserTest do end end - describe "github_associate_changeset" do - test "should cast github_id" do + describe "github_association_changeset" do + test "should cast github_auth_token" do user = insert(:user) - changeset = user |> User.github_associate_changeset(%{github_id: "foobar"}) + changeset = user |> User.github_association_changeset(%{github_auth_token: "foobar"}) assert changeset.valid? - assert changeset.changes.github_id == "foobar" + assert changeset.changes.github_auth_token == "foobar" end - test "github_id should be required" do + test "github_auth_token should be required" do user = insert(:user) - changeset = user |> User.github_associate_changeset(%{}) + changeset = user |> User.github_association_changeset(%{}) refute changeset.valid? end end diff --git a/web/controllers/user_controller.ex b/web/controllers/user_controller.ex index 038b4c465..6cb7d769a 100644 --- a/web/controllers/user_controller.ex +++ b/web/controllers/user_controller.ex @@ -35,7 +35,7 @@ defmodule CodeCorps.UserController do end end - def github_connect(conn, code) do + def github_connect(conn, %{"code" => code}) do current_user = Guardian.Plug.current_resource(conn) with {:ok, user} <- Github.connect(current_user, code) do diff --git a/web/models/project.ex b/web/models/project.ex index a57532099..bdf63c6dd 100644 --- a/web/models/project.ex +++ b/web/models/project.ex @@ -13,6 +13,8 @@ defmodule CodeCorps.Project do alias CodeCorps.Services.MarkdownRendererService alias CodeCorps.TaskList + alias Ecto.Changeset + @type t :: %__MODULE__{} schema "projects" do diff --git a/web/models/user.ex b/web/models/user.ex index cd9ec0797..969ef4e55 100644 --- a/web/models/user.ex +++ b/web/models/user.ex @@ -30,10 +30,12 @@ defmodule CodeCorps.User do field :twitter, :string field :username, :string field :website, :string - field :github_id, :string field :state, :string, default: "signed_up" field :state_transition, :string, virtual: true + field :github_id, :string + field :github_auth_token, :string + has_one :slugged_route, SluggedRoute has_many :project_users, CodeCorps.ProjectUser @@ -95,10 +97,10 @@ defmodule CodeCorps.User do |> apply_state_transition(struct) end - def github_associate_changeset(struct, params) do + def github_association_changeset(struct, params) do struct - |> cast(params, [:github_id]) - |> validate_required([:github_id]) + |> cast(params, [:github_auth_token]) + |> validate_required([:github_auth_token]) end def reset_password_changeset(struct, params) do From 00b565bdf12f0432bf8a39e3c9638bc97f200bea Mon Sep 17 00:00:00 2001 From: Josh Smith Date: Wed, 31 May 2017 16:40:03 -0700 Subject: [PATCH 2/4] Fix GitHub OAuth connect flow --- .env.example | 4 ++-- blueprint/api.apib | 4 ++-- config/dev.exs | 6 +++--- config/prod.exs | 4 ++-- config/remote-development.exs | 4 ++-- config/staging.exs | 4 ++-- config/test.exs | 2 +- lib/code_corps/github.ex | 16 ++++++++-------- lib/code_corps/github/api.ex | 18 +++++++++--------- lib/code_corps/github/api_contract.ex | 18 +++++++++++------- lib/code_corps/github/test_api.ex | 6 +++--- priv/repo/structure.sql | 1 + test/controllers/user_controller_test.exs | 6 +++--- test/lib/code_corps/github_test.exs | 16 ++++++++-------- web/controllers/user_controller.ex | 11 +++++++---- web/router.ex | 2 +- 16 files changed, 65 insertions(+), 57 deletions(-) diff --git a/.env.example b/.env.example index 1ae8373fd..ad816e744 100644 --- a/.env.example +++ b/.env.example @@ -4,8 +4,8 @@ export CLOUDEX_API_KEY= export CLOUDEX_CLOUD_NAME= export CLOUDEX_SECRET= export CLOUDFRONT_DOMAIN= -export GITHUB_CLIENT_ID= -export GITHUB_CLIENT_SECRET= +export GITHUB_OAUTH_CLIENT_ID= +export GITHUB_OAUTH_CLIENT_SECRET= export POSTMARK_API_KEY= export S3_BUCKET= export SEGMENT_WRITE_KEY= diff --git a/blueprint/api.apib b/blueprint/api.apib index 482bbffd6..c86e99f18 100644 --- a/blueprint/api.apib +++ b/blueprint/api.apib @@ -2306,9 +2306,9 @@ This endpoint handles the User resources for Code Corps. + Attributes (Record Not Found Response) -### Connect user to GitHub [POST /github_connect] +### Connect user to GitHub [POST /oauth/github] -This endpoint allows a user to connect their CodeCorps account to their respective GitHub account. +This endpoint allows a user to connect their Code Corps account to their respective GitHub account. + Request diff --git a/config/dev.exs b/config/dev.exs index 74e0300cf..70cbcad6e 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -49,7 +49,7 @@ config :guardian, Guardian, config :code_corps, :analytics, CodeCorps.Analytics.InMemoryAPI -config :code_corps, :github_api, CodeCorps.Github.API +config :code_corps, :github_api, CodeCorps.GitHub.API # Configures stripe for dev mode config :code_corps, :stripe, Stripe @@ -75,5 +75,5 @@ if System.get_env("CLOUDEX_API_KEY") == nil do end config :code_corps, - github_client_id: System.get_env("GITHUB_CLIENT_ID"), - github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + github_oauth_client_id: System.get_env("GITHUB_OAUTH_CLIENT_ID"), + github_oauth_client_secret: System.get_env("GITHUB_OAUTH_CLIENT_SECRET") diff --git a/config/prod.exs b/config/prod.exs index c5d375f7c..9c9b1d7e5 100644 --- a/config/prod.exs +++ b/config/prod.exs @@ -66,8 +66,8 @@ config :code_corps, postmark_receipt_template: "1255222" config :code_corps, - github_client_id: System.get_env("GITHUB_CLIENT_ID"), - github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + github_oauth_client_id: System.get_env("GITHUB_OAUTH_CLIENT_ID"), + github_oauth_client_secret: System.get_env("GITHUB_OAUTH_CLIENT_SECRET") # ## SSL Support # diff --git a/config/remote-development.exs b/config/remote-development.exs index 4df1d6924..e3245e524 100644 --- a/config/remote-development.exs +++ b/config/remote-development.exs @@ -48,8 +48,8 @@ config :code_corps, postmark_receipt_template: "123" config :code_corps, - github_client_id: System.get_env("GITHUB_CLIENT_ID"), - github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + github_oauth_client_id: System.get_env("GITHUB_OAUTH_CLIENT_ID"), + github_oauth_client_secret: System.get_env("GITHUB_OAUTH_CLIENT_SECRET") # ## SSL Support # diff --git a/config/staging.exs b/config/staging.exs index 8fd031fd9..003103e37 100644 --- a/config/staging.exs +++ b/config/staging.exs @@ -64,8 +64,8 @@ config :code_corps, postmark_receipt_template: "1252361" config :code_corps, - github_client_id: System.get_env("GITHUB_CLIENT_ID"), - github_client_secret: System.get_env("GITHUB_CLIENT_SECRET") + github_oauth_client_id: System.get_env("GITHUB_OAUTH_CLIENT_ID"), + github_oauth_client_secret: System.get_env("GITHUB_OAUTH_CLIENT_SECRET") # ## SSL Support # diff --git a/config/test.exs b/config/test.exs index e6a3a2be5..8019cc09b 100644 --- a/config/test.exs +++ b/config/test.exs @@ -32,7 +32,7 @@ config :guardian, Guardian, config :code_corps, :analytics, CodeCorps.Analytics.TestAPI -config :code_corps, :github_api, CodeCorps.Github.TestAPI +config :code_corps, :github_api, CodeCorps.GitHub.TestAPI # Configures stripe for test mode config :code_corps, :stripe, CodeCorps.StripeTesting diff --git a/lib/code_corps/github.ex b/lib/code_corps/github.ex index 1101dc04e..c0d993c7c 100644 --- a/lib/code_corps/github.ex +++ b/lib/code_corps/github.ex @@ -1,20 +1,20 @@ -defmodule CodeCorps.Github do +defmodule CodeCorps.GitHub do alias CodeCorps.{User, Repo} @api Application.get_env(:code_corps, :github_api) @doc """ - Posts code to github to receive an auth token, associates user with that - auth token. + POSTs `code` to GitHub to receive an OAuth token, then associates the user + with that OAuth token. - Accepts a third parameter, which is a custom API module, for the purposes of + Accepts a third parameter – a custom API module – for the purposes of explicit dependency injection during testing. Returns one of the following: - - {:ok, %CodeCorps.User{}} - - {:error, %Ecto.Changeset{}} - - {:error, "some_github_error"} + - `{:ok, %CodeCorps.User{}}` + - `{:error, %Ecto.Changeset{}}` + - `{:error, "some_github_error"}` """ @spec connect(User.t, String.t, module) :: {:ok, User.t} | {:error, String.t} def connect(%User{} = user, code, api \\ @api) do @@ -25,7 +25,7 @@ defmodule CodeCorps.Github do end @doc """ - Associates user with an auth token + Associates user with the GitHub OAuth token. Returns one of the following: diff --git a/lib/code_corps/github/api.ex b/lib/code_corps/github/api.ex index f61107fe3..c46b93629 100644 --- a/lib/code_corps/github/api.ex +++ b/lib/code_corps/github/api.ex @@ -1,12 +1,12 @@ -defmodule CodeCorps.Github.API do +defmodule CodeCorps.GitHub.API do @moduledoc """ - The boundary module which communicates with the Github API using either - direct requests, or through Tentacat + The boundary module which communicates with the GitHub API using either + direct requests, or through Tentacat. """ - @behaviour CodeCorps.Github.APIContract + @behaviour CodeCorps.GitHub.APIContract - @client_secret Application.get_env(:code_corps, :github_client_secret) - @client_id Application.get_env(:code_corps, :github_client_id) + @client_secret Application.get_env(:code_corps, :github_oauth_client_secret) + @client_id Application.get_env(:code_corps, :github_oauth_client_id) @base_connect_params %{ client_id: @client_id, @@ -14,8 +14,8 @@ defmodule CodeCorps.Github.API do } @doc """ - Receives a code generated through the client-side github connect process and - posts it to github. + Receives a code generated through the client-side GitHub connect process + and posts it to GitHub. Returns either an {:ok, access_token}, or an {:error, error_message}. """ @@ -34,7 +34,7 @@ defmodule CodeCorps.Github.API do @spec do_connect(map) :: {:ok, HTTPoison.Response.t | HTTPoison.AsyncResponse.t} | {:error, HTTPoison.Error.t} defp do_connect(params) do - HTTPoison.post(@connect_url, "", [{"Accept", "application/json"}], [params: params]) + HTTPoison.post(@connect_url, "", [{"Accept", "application/json"}, {"Content-Type", "application/json"}], [params: params]) end @spec build_connect_params(String.t) :: map diff --git a/lib/code_corps/github/api_contract.ex b/lib/code_corps/github/api_contract.ex index deb1aa4f5..e18ec6bc5 100644 --- a/lib/code_corps/github/api_contract.ex +++ b/lib/code_corps/github/api_contract.ex @@ -1,17 +1,21 @@ -defmodule CodeCorps.Github.APIContract do +defmodule CodeCorps.GitHub.APIContract do @moduledoc """ - Defines a contract for a github API module, listing all functions the module + Defines a contract for a GitHub API module, listing all functions the module should implement. - This contract should be specified as behaviour for the default `Github.API` + This contract should be specified as behaviour for the default `GitHub.API` module, as well as any custom module we inject in tests. """ @doc """ - Receives a code string, created in the client part of the github connect process, - returns either an :ok tupple indicating a successful connect process, where - the second element is the auth token string, or an :error tuple, where the - second element is an error message, or a struct + Receives a code string, created on the client side of the GitHub connect + process. + + Returns one of: + + - an `:ok` tuple indicating a successful connect process, where + the second element is the OAuth token string + - an `:error` tuple, where the second element is an error message or a struct """ @callback connect(code :: String.t) :: {:ok, auth_token :: String.t} | {:error, error :: String.t} end diff --git a/lib/code_corps/github/test_api.ex b/lib/code_corps/github/test_api.ex index d248591be..4e13a615c 100644 --- a/lib/code_corps/github/test_api.ex +++ b/lib/code_corps/github/test_api.ex @@ -1,12 +1,12 @@ -defmodule CodeCorps.Github.TestAPI do +defmodule CodeCorps.GitHub.TestAPI do @moduledoc """ The most basic implementation of an API module for testing. All functions - here should return successes. + here should return `:ok` tuples. If we want to test the wrapper module, we can specify custom API modules during function calls. """ - @behaviour CodeCorps.Github.APIContract + @behaviour CodeCorps.GitHub.APIContract @spec connect(String.t) :: {:ok, String.t} def connect(code), do: send(self(), {:ok, code}) diff --git a/priv/repo/structure.sql b/priv/repo/structure.sql index 56a1ba05b..1b88c1d93 100644 --- a/priv/repo/structure.sql +++ b/priv/repo/structure.sql @@ -2600,3 +2600,4 @@ ALTER TABLE ONLY user_tasks -- INSERT INTO "schema_migrations" (version) VALUES (20160723215749), (20160804000000), (20160804001111), (20160805132301), (20160805203929), (20160808143454), (20160809214736), (20160810124357), (20160815125009), (20160815143002), (20160816020347), (20160816034021), (20160817220118), (20160818000944), (20160818132546), (20160820113856), (20160820164905), (20160822002438), (20160822004056), (20160822011624), (20160822020401), (20160822044612), (20160830081224), (20160830224802), (20160911233738), (20160912002705), (20160912145957), (20160918003206), (20160928232404), (20161003185918), (20161019090945), (20161019110737), (20161020144622), (20161021131026), (20161031001615), (20161121005339), (20161121014050), (20161121043941), (20161121045709), (20161122015942), (20161123081114), (20161123150943), (20161124085742), (20161125200620), (20161126045705), (20161127054559), (20161205024856), (20161207112519), (20161209192504), (20161212005641), (20161214005935), (20161215052051), (20161216051447), (20161218005913), (20161219160401), (20161219163909), (20161220141753), (20161221085759), (20161226213600), (20161231063614), (20170102130055), (20170102181053), (20170104113708), (20170104212623), (20170104235423), (20170106013143), (20170115035159), (20170115230549), (20170121014100), (20170131234029), (20170201014901), (20170201025454), (20170201035458), (20170201183258), (20170220032224), (20170224233516), (20170226050552), (20170228085250), (20170308214128), (20170308220713), (20170308222552), (20170313130611), (20170318032449), (20170318082740), (20170324194827), (20170424215355), (20170501225441), (20170526095401); + diff --git a/test/controllers/user_controller_test.exs b/test/controllers/user_controller_test.exs index 9baed8590..5de0eb2e1 100644 --- a/test/controllers/user_controller_test.exs +++ b/test/controllers/user_controller_test.exs @@ -242,13 +242,13 @@ defmodule CodeCorps.UserControllerTest do end end - describe "github_connect" do + describe "github_oauth" do test "return the user when current user connects successfully", %{conn: conn} do user = insert(:user) code = %{"code" => "valid_code"} - path = user_path(conn, :github_connect, code) + path = user_path(conn, :github_oauth, code) json = conn |> authenticate(user) |> post(path) |> json_response(200) @@ -258,7 +258,7 @@ defmodule CodeCorps.UserControllerTest do test "return unauthenticated error code when no current user", %{conn: conn} do code = %{"code" => "client generated code"} - path = user_path(conn, :github_connect, code) + path = user_path(conn, :github_oauth, code) conn |> post(path) |> json_response(401) end diff --git a/test/lib/code_corps/github_test.exs b/test/lib/code_corps/github_test.exs index c8a1a8657..4777fe26c 100644 --- a/test/lib/code_corps/github_test.exs +++ b/test/lib/code_corps/github_test.exs @@ -1,8 +1,8 @@ -defmodule CodeCorps.GithubTest do +defmodule CodeCorps.GitHubTest do use CodeCorps.ModelCase alias CodeCorps.{ - Github, User + GitHub, User } alias Ecto.Changeset @@ -11,7 +11,7 @@ defmodule CodeCorps.GithubTest do test "updates the user, returns :ok tuple with user" do user = insert(:user) params = %{github_auth_token: "foobar"} - {:ok, %User{} = returned_user} = Github.associate(user, params) + {:ok, %User{} = returned_user} = GitHub.associate(user, params) assert user.id == returned_user.id assert returned_user.github_auth_token == "foobar" end @@ -19,18 +19,18 @@ defmodule CodeCorps.GithubTest do test "returns :error tupple with changeset if there are validation errors" do user = insert(:user) params = %{} - {:error, %Changeset{} = changeset} = Github.associate(user, params) + {:error, %Changeset{} = changeset} = GitHub.associate(user, params) refute changeset.valid? end end defmodule SuccessAPI do - @behaviour CodeCorps.Github.APIContract + @behaviour CodeCorps.GitHub.APIContract def connect(_code), do: {:ok, "foo_auth_token"} end defmodule ErrorAPI do - @behaviour CodeCorps.Github.APIContract + @behaviour CodeCorps.GitHub.APIContract def connect(_code), do: {:error, "foo_error"} end @@ -38,7 +38,7 @@ defmodule CodeCorps.GithubTest do test "posts to github, updates user if reply is ok, returns updated user" do user = insert(:user) - {:ok, %User{} = returned_user} = Github.connect(user, "foo", SuccessAPI) + {:ok, %User{} = returned_user} = GitHub.connect(user, "foo", SuccessAPI) assert returned_user.id == user.id assert returned_user.github_auth_token == "foo_auth_token" @@ -46,7 +46,7 @@ defmodule CodeCorps.GithubTest do test "posts to github, returns error if reply is not ok" do user = insert(:user) - assert {:error, "foo_error"} == Github.connect(user, "foo", ErrorAPI) + assert {:error, "foo_error"} == GitHub.connect(user, "foo", ErrorAPI) end end end diff --git a/web/controllers/user_controller.ex b/web/controllers/user_controller.ex index 6cb7d769a..63396d59b 100644 --- a/web/controllers/user_controller.ex +++ b/web/controllers/user_controller.ex @@ -6,7 +6,7 @@ defmodule CodeCorps.UserController do alias CodeCorps.User alias CodeCorps.Services.UserService - alias CodeCorps.Github + alias CodeCorps.GitHub plug :load_and_authorize_resource, model: User, only: [:update] plug JaResource @@ -35,11 +35,14 @@ defmodule CodeCorps.UserController do end end - def github_connect(conn, %{"code" => code}) do + @doc """ + Differs from other resources by path: `/oauth/github` + """ + def github_oauth(conn, %{"code" => code}) do current_user = Guardian.Plug.current_resource(conn) - with {:ok, user} <- Github.connect(current_user, code) + with {:ok, user} <- GitHub.connect(current_user, code) do - conn |> render(:show, data: user) + conn |> render("show.json-api", data: user) else {:error, %Ecto.Changeset{} = changeset} -> conn diff --git a/web/router.ex b/web/router.ex index 5613e99a2..4db1f1fde 100644 --- a/web/router.ex +++ b/web/router.ex @@ -63,7 +63,7 @@ defmodule CodeCorps.Router do resources "/categories", CategoryController, only: [:create, :update] resources "/comments", CommentController, only: [:create, :update] resources "/donation-goals", DonationGoalController, only: [:create, :update, :delete] - post "/github-connect", UserController, :github_connect + post "/oauth/github", UserController, :github_oauth resources "/organizations", OrganizationController, only: [:create, :update] resources "/previews", PreviewController, only: [:create] resources "/project-categories", ProjectCategoryController, only: [:create, :delete] From 68590672810e477b733fda982e1460328035c448 Mon Sep 17 00:00:00 2001 From: Jessica Jordan Date: Sun, 4 Jun 2017 19:24:52 +0200 Subject: [PATCH 3/4] Adds failing test This adds a test case for checking number validation of the DonationGoal's :amount attribute when a new donation goal is created or an exisiting one is updated. --- test/models/donation_goal_test.exs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/models/donation_goal_test.exs b/test/models/donation_goal_test.exs index 07bb9e768..c8d420ddb 100644 --- a/test/models/donation_goal_test.exs +++ b/test/models/donation_goal_test.exs @@ -26,6 +26,15 @@ defmodule CodeCorps.DonationGoalTest do refute changeset.valid? changeset |> assert_error_message(:project, "does not exist") end + + test "amount must not be negative" do + attrs = %{amount: -100, description: "Cashback for donators", project_id: 2} + donation_goal = insert(:donation_goal) + changeset = DonationGoal.create_changeset(donation_goal, attrs) + + refute changeset.valid? + changeset |> assert_error_message(:amount, "must be greater than %{number}") + end end describe "&update_changeset/2" do From 85684d08ce5be595b08ae4e0a74c653fca5c99b2 Mon Sep 17 00:00:00 2001 From: Jessica Jordan Date: Sun, 4 Jun 2017 19:30:41 +0200 Subject: [PATCH 4/4] Adds number validation for the DonationGoal's amount This adds a validation for the amount attribute of donation goals to be greater than zero. --- web/models/donation_goal.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/web/models/donation_goal.ex b/web/models/donation_goal.ex index 572ef430f..375c431d0 100644 --- a/web/models/donation_goal.ex +++ b/web/models/donation_goal.ex @@ -30,6 +30,7 @@ defmodule CodeCorps.DonationGoal do struct |> cast(params, [:amount, :description, :project_id]) |> validate_required([:amount, :description, :project_id]) + |> validate_number(:amount, greater_than: 0) |> assoc_constraint(:project) end